thr3ads.net - Virtualization - [PATCH net-next v3] virtio_net: Introduce skb_vnet_common

If this information is useful, please help other people find it:
Share via:

Feng Liu

2023-Aug-21 14:27 UTC

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

The virtio_net driver currently deals with different versions and types
of virtio net headers, such as virtio_net_hdr_mrg_rxbuf,
virtio_net_hdr_v1_hash, etc. Due to these variations, the code relies
on multiple type casts to convert memory between different structures,
potentially leading to bugs when there are changes in these structures.

Introduces the "struct skb_vnet_common_hdr" as a unifying header
structure using a union. With this approach, various virtio net header
structures can be converted by accessing different members of this
structure, thus eliminating the need for type casting and reducing the
risk of potential bugs.

For example following code:
static struct sk_buff *page_to_skb(struct virtnet_info *vi,
		struct receive_queue *rq,
		struct page *page, unsigned int offset,
		unsigned int len, unsigned int truesize,
		unsigned int headroom)
{
[...]
	struct virtio_net_hdr_mrg_rxbuf *hdr;
[...]
	hdr_len = vi->hdr_len;
[...]
ok:
	hdr = skb_vnet_hdr(skb);
	memcpy(hdr, hdr_p, hdr_len);
[...]
}

When VIRTIO_NET_F_HASH_REPORT feature is enabled, hdr_len = 20
But the sizeof(*hdr) is 12,
memcpy(hdr, hdr_p, hdr_len); will copy 20 bytes to the hdr,
which make a potential risk of bug. And this risk can be avoided by
introducing struct skb_vnet_common_hdr.

Change log
v1->v2
feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
feedback from Simon Horman <horms at kernel.org>
1. change to use net-next tree.
2. move skb_vnet_common_hdr inside kernel file instead of the UAPI header.

v2->v3
feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
1. fix typo in commit message.
2. add original struct virtio_net_hdr into union
3. remove virtio_net_hdr_mrg_rxbuf variable in receive_buf;

Signed-off-by: Feng Liu <feliu at nvidia.com>
Reviewed-by: Jiri Pirko <jiri at nvidia.com>
---
 drivers/net/virtio_net.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8e9f4cfe941f..8c74bc8cfe68 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -303,6 +303,14 @@ struct padded_vnet_hdr {
 	char padding[12];
 };
 
+struct virtio_net_common_hdr {
+	union {
+		struct virtio_net_hdr hdr;
+		struct virtio_net_hdr_mrg_rxbuf	mrg_hdr;
+		struct virtio_net_hdr_v1_hash hash_v1_hdr;
+	};
+};
+
 static void virtnet_rq_free_unused_buf(struct virtqueue *vq, void *buf);
 static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
 
@@ -344,9 +352,10 @@ static int rxq2vq(int rxq)
 	return rxq * 2;
 }
 
-static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff
*skb)
+static inline struct virtio_net_common_hdr *
+skb_vnet_common_hdr(struct sk_buff *skb)
 {
-	return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb;
+	return (struct virtio_net_common_hdr *)skb->cb;
 }
 
 /*
@@ -469,7 +478,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 				   unsigned int headroom)
 {
 	struct sk_buff *skb;
-	struct virtio_net_hdr_mrg_rxbuf *hdr;
+	struct virtio_net_common_hdr *hdr;
 	unsigned int copy, hdr_len, hdr_padded_len;
 	struct page *page_to_free = NULL;
 	int tailroom, shinfo_size;
@@ -554,7 +563,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 		give_pages(rq, page);
 
 ok:
-	hdr = skb_vnet_hdr(skb);
+	hdr = skb_vnet_common_hdr(skb);
 	memcpy(hdr, hdr_p, hdr_len);
 	if (page_to_free)
 		put_page(page_to_free);
@@ -966,7 +975,7 @@ static struct sk_buff *receive_small_build_skb(struct
virtnet_info *vi,
 		return NULL;
 
 	buf += header_offset;
-	memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
+	memcpy(skb_vnet_common_hdr(skb), buf, vi->hdr_len);
 
 	return skb;
 }
@@ -1577,7 +1586,7 @@ static void receive_buf(struct virtnet_info *vi, struct
receive_queue *rq,
 {
 	struct net_device *dev = vi->dev;
 	struct sk_buff *skb;
-	struct virtio_net_hdr_mrg_rxbuf *hdr;
+	struct virtio_net_common_hdr *hdr;
 
 	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
 		pr_debug("%s: short packet %i\n", dev->name, len);
@@ -1597,9 +1606,9 @@ static void receive_buf(struct virtnet_info *vi, struct
receive_queue *rq,
 	if (unlikely(!skb))
 		return;
 
-	hdr = skb_vnet_hdr(skb);
+	hdr = skb_vnet_common_hdr(skb);
 	if (dev->features & NETIF_F_RXHASH &&
vi->has_rss_hash_report)
-		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
+		virtio_skb_set_hash(&hdr->hash_v1_hdr, skb);
 
 	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -2105,7 +2114,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff
*skb)
 	if (can_push)
 		hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
 	else
-		hdr = skb_vnet_hdr(skb);
+		hdr = &skb_vnet_common_hdr(skb)->mrg_hdr;
 
 	if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
 				    virtio_is_little_endian(vi->vdev), false,
-- 
2.37.1 (Apple Git-137.1)

Willem de Bruijn

2023-Aug-21 14:46 UTC

head link

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

Feng Liu wrote:> The virtio_net driver currently deals with different versions and types
> of virtio net headers, such as virtio_net_hdr_mrg_rxbuf,
> virtio_net_hdr_v1_hash, etc. Due to these variations, the code relies
> on multiple type casts to convert memory between different structures,
> potentially leading to bugs when there are changes in these structures.
> 
> Introduces the "struct skb_vnet_common_hdr" as a unifying header
> structure using a union. With this approach, various virtio net header
> structures can be converted by accessing different members of this
> structure, thus eliminating the need for type casting and reducing the
> risk of potential bugs.
> 
> For example following code:
> static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> 		struct receive_queue *rq,
> 		struct page *page, unsigned int offset,
> 		unsigned int len, unsigned int truesize,
> 		unsigned int headroom)
> {
> [...]
> 	struct virtio_net_hdr_mrg_rxbuf *hdr;
> [...]
> 	hdr_len = vi->hdr_len;
> [...]
> ok:
> 	hdr = skb_vnet_hdr(skb);
> 	memcpy(hdr, hdr_p, hdr_len);
> [...]
> }
> 
> When VIRTIO_NET_F_HASH_REPORT feature is enabled, hdr_len = 20
> But the sizeof(*hdr) is 12,
> memcpy(hdr, hdr_p, hdr_len); will copy 20 bytes to the hdr,
> which make a potential risk of bug. And this risk can be avoided by
> introducing struct skb_vnet_common_hdr.
> 
> Change log
> v1->v2
> feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
> feedback from Simon Horman <horms at kernel.org>
> 1. change to use net-next tree.
> 2. move skb_vnet_common_hdr inside kernel file instead of the UAPI header.
> 
> v2->v3
> feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
> 1. fix typo in commit message.
> 2. add original struct virtio_net_hdr into union
> 3. remove virtio_net_hdr_mrg_rxbuf variable in receive_buf;
> 
> Signed-off-by: Feng Liu <feliu at nvidia.com>
> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
Reviewed-by: Willem de Bruijn <willemb at google.com>

A similar solution as tpacket_uhdr.

Feng Liu

2023-Aug-22 14:25 UTC

head link

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

Hi Jason , MST

Could you help to review this patch ?  Any more comments?

Thanks
Feng

On 2023-08-21 a.m.10:27, Feng Liu wrote:> The virtio_net driver currently deals with different versions and types
> of virtio net headers, such as virtio_net_hdr_mrg_rxbuf,
> virtio_net_hdr_v1_hash, etc. Due to these variations, the code relies
> on multiple type casts to convert memory between different structures,
> potentially leading to bugs when there are changes in these structures.
> 
> Introduces the "struct skb_vnet_common_hdr" as a unifying header
> structure using a union. With this approach, various virtio net header
> structures can be converted by accessing different members of this
> structure, thus eliminating the need for type casting and reducing the
> risk of potential bugs.
> 
> For example following code:
> static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> 		struct receive_queue *rq,
> 		struct page *page, unsigned int offset,
> 		unsigned int len, unsigned int truesize,
> 		unsigned int headroom)
> {
> [...]
> 	struct virtio_net_hdr_mrg_rxbuf *hdr;
> [...]
> 	hdr_len = vi->hdr_len;
> [...]
> ok:
> 	hdr = skb_vnet_hdr(skb);
> 	memcpy(hdr, hdr_p, hdr_len);
> [...]
> }
> 
> When VIRTIO_NET_F_HASH_REPORT feature is enabled, hdr_len = 20
> But the sizeof(*hdr) is 12,
> memcpy(hdr, hdr_p, hdr_len); will copy 20 bytes to the hdr,
> which make a potential risk of bug. And this risk can be avoided by
> introducing struct skb_vnet_common_hdr.
> 
> Change log
> v1->v2
> feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
> feedback from Simon Horman <horms at kernel.org>
> 1. change to use net-next tree.
> 2. move skb_vnet_common_hdr inside kernel file instead of the UAPI header.
> 
> v2->v3
> feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
> 1. fix typo in commit message.
> 2. add original struct virtio_net_hdr into union
> 3. remove virtio_net_hdr_mrg_rxbuf variable in receive_buf;
> 
> Signed-off-by: Feng Liu <feliu at nvidia.com>
> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
> ---
>   drivers/net/virtio_net.c | 27 ++++++++++++++++++---------
>   1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8e9f4cfe941f..8c74bc8cfe68 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -303,6 +303,14 @@ struct padded_vnet_hdr {
>   	char padding[12];
>   };
>   
> +struct virtio_net_common_hdr {
> +	union {
> +		struct virtio_net_hdr hdr;
> +		struct virtio_net_hdr_mrg_rxbuf	mrg_hdr;
> +		struct virtio_net_hdr_v1_hash hash_v1_hdr;
> +	};
> +};
> +
>   static void virtnet_rq_free_unused_buf(struct virtqueue *vq, void *buf);
>   static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
>   
> @@ -344,9 +352,10 @@ static int rxq2vq(int rxq)
>   	return rxq * 2;
>   }
>   
> -static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff
*skb)
> +static inline struct virtio_net_common_hdr *
> +skb_vnet_common_hdr(struct sk_buff *skb)
>   {
> -	return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb;
> +	return (struct virtio_net_common_hdr *)skb->cb;
>   }
>   
>   /*
> @@ -469,7 +478,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
>   				   unsigned int headroom)
>   {
>   	struct sk_buff *skb;
> -	struct virtio_net_hdr_mrg_rxbuf *hdr;
> +	struct virtio_net_common_hdr *hdr;
>   	unsigned int copy, hdr_len, hdr_padded_len;
>   	struct page *page_to_free = NULL;
>   	int tailroom, shinfo_size;
> @@ -554,7 +563,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
>   		give_pages(rq, page);
>   
>   ok:
> -	hdr = skb_vnet_hdr(skb);
> +	hdr = skb_vnet_common_hdr(skb);
>   	memcpy(hdr, hdr_p, hdr_len);
>   	if (page_to_free)
>   		put_page(page_to_free);
> @@ -966,7 +975,7 @@ static struct sk_buff *receive_small_build_skb(struct
virtnet_info *vi,
>   		return NULL;
>   
>   	buf += header_offset;
> -	memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> +	memcpy(skb_vnet_common_hdr(skb), buf, vi->hdr_len);
>   
>   	return skb;
>   }
> @@ -1577,7 +1586,7 @@ static void receive_buf(struct virtnet_info *vi,
struct receive_queue *rq,
>   {
>   	struct net_device *dev = vi->dev;
>   	struct sk_buff *skb;
> -	struct virtio_net_hdr_mrg_rxbuf *hdr;
> +	struct virtio_net_common_hdr *hdr;
>   
>   	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
>   		pr_debug("%s: short packet %i\n", dev->name, len);
> @@ -1597,9 +1606,9 @@ static void receive_buf(struct virtnet_info *vi,
struct receive_queue *rq,
>   	if (unlikely(!skb))
>   		return;
>   
> -	hdr = skb_vnet_hdr(skb);
> +	hdr = skb_vnet_common_hdr(skb);
>   	if (dev->features & NETIF_F_RXHASH &&
vi->has_rss_hash_report)
> -		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> +		virtio_skb_set_hash(&hdr->hash_v1_hdr, skb);
>   
>   	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
>   		skb->ip_summed = CHECKSUM_UNNECESSARY;
> @@ -2105,7 +2114,7 @@ static int xmit_skb(struct send_queue *sq, struct
sk_buff *skb)
>   	if (can_push)
>   		hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
>   	else
> -		hdr = skb_vnet_hdr(skb);
> +		hdr = &skb_vnet_common_hdr(skb)->mrg_hdr;
>   
>   	if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
>   				    virtio_is_little_endian(vi->vdev), false,

Jason Wang

2023-Aug-23 00:36 UTC

head link

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

On Mon, Aug 21, 2023 at 10:27?PM Feng Liu <feliu at nvidia.com>
wrote:>
> The virtio_net driver currently deals with different versions and types
> of virtio net headers, such as virtio_net_hdr_mrg_rxbuf,
> virtio_net_hdr_v1_hash, etc. Due to these variations, the code relies
> on multiple type casts to convert memory between different structures,
> potentially leading to bugs when there are changes in these structures.
>
> Introduces the "struct skb_vnet_common_hdr" as a unifying header
> structure using a union. With this approach, various virtio net header
> structures can be converted by accessing different members of this
> structure, thus eliminating the need for type casting and reducing the
> risk of potential bugs.
>
> For example following code:
> static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>                 struct receive_queue *rq,
>                 struct page *page, unsigned int offset,
>                 unsigned int len, unsigned int truesize,
>                 unsigned int headroom)
> {
> [...]
>         struct virtio_net_hdr_mrg_rxbuf *hdr;
> [...]
>         hdr_len = vi->hdr_len;
> [...]
> ok:
>         hdr = skb_vnet_hdr(skb);
>         memcpy(hdr, hdr_p, hdr_len);
> [...]
> }
>
> When VIRTIO_NET_F_HASH_REPORT feature is enabled, hdr_len = 20
> But the sizeof(*hdr) is 12,
> memcpy(hdr, hdr_p, hdr_len); will copy 20 bytes to the hdr,
> which make a potential risk of bug. And this risk can be avoided by
> introducing struct skb_vnet_common_hdr.
>
> Change log
> v1->v2
> feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
> feedback from Simon Horman <horms at kernel.org>
> 1. change to use net-next tree.
> 2. move skb_vnet_common_hdr inside kernel file instead of the UAPI header.
>
> v2->v3
> feedback from Willem de Bruijn <willemdebruijn.kernel at gmail.com>
> 1. fix typo in commit message.
> 2. add original struct virtio_net_hdr into union
> 3. remove virtio_net_hdr_mrg_rxbuf variable in receive_buf;
>
> Signed-off-by: Feng Liu <feliu at nvidia.com>
> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
Acked-by: Jason Wang <jasowang at redhat.com>

Thanks
> ---
>  drivers/net/virtio_net.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8e9f4cfe941f..8c74bc8cfe68 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -303,6 +303,14 @@ struct padded_vnet_hdr {
>         char padding[12];
>  };
>
> +struct virtio_net_common_hdr {
> +       union {
> +               struct virtio_net_hdr hdr;
> +               struct virtio_net_hdr_mrg_rxbuf mrg_hdr;
> +               struct virtio_net_hdr_v1_hash hash_v1_hdr;
> +       };
> +};
> +
>  static void virtnet_rq_free_unused_buf(struct virtqueue *vq, void *buf);
>  static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
>
> @@ -344,9 +352,10 @@ static int rxq2vq(int rxq)
>         return rxq * 2;
>  }
>
> -static inline struct virtio_net_hdr_mrg_rxbuf *skb_vnet_hdr(struct sk_buff
*skb)
> +static inline struct virtio_net_common_hdr *
> +skb_vnet_common_hdr(struct sk_buff *skb)
>  {
> -       return (struct virtio_net_hdr_mrg_rxbuf *)skb->cb;
> +       return (struct virtio_net_common_hdr *)skb->cb;
>  }
>
>  /*
> @@ -469,7 +478,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
>                                    unsigned int headroom)
>  {
>         struct sk_buff *skb;
> -       struct virtio_net_hdr_mrg_rxbuf *hdr;
> +       struct virtio_net_common_hdr *hdr;
>         unsigned int copy, hdr_len, hdr_padded_len;
>         struct page *page_to_free = NULL;
>         int tailroom, shinfo_size;
> @@ -554,7 +563,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info
*vi,
>                 give_pages(rq, page);
>
>  ok:
> -       hdr = skb_vnet_hdr(skb);
> +       hdr = skb_vnet_common_hdr(skb);
>         memcpy(hdr, hdr_p, hdr_len);
>         if (page_to_free)
>                 put_page(page_to_free);
> @@ -966,7 +975,7 @@ static struct sk_buff *receive_small_build_skb(struct
virtnet_info *vi,
>                 return NULL;
>
>         buf += header_offset;
> -       memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> +       memcpy(skb_vnet_common_hdr(skb), buf, vi->hdr_len);
>
>         return skb;
>  }
> @@ -1577,7 +1586,7 @@ static void receive_buf(struct virtnet_info *vi,
struct receive_queue *rq,
>  {
>         struct net_device *dev = vi->dev;
>         struct sk_buff *skb;
> -       struct virtio_net_hdr_mrg_rxbuf *hdr;
> +       struct virtio_net_common_hdr *hdr;
>
>         if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
>                 pr_debug("%s: short packet %i\n", dev->name,
len);
> @@ -1597,9 +1606,9 @@ static void receive_buf(struct virtnet_info *vi,
struct receive_queue *rq,
>         if (unlikely(!skb))
>                 return;
>
> -       hdr = skb_vnet_hdr(skb);
> +       hdr = skb_vnet_common_hdr(skb);
>         if (dev->features & NETIF_F_RXHASH &&
vi->has_rss_hash_report)
> -               virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash
*)hdr, skb);
> +               virtio_skb_set_hash(&hdr->hash_v1_hdr, skb);
>
>         if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
>                 skb->ip_summed = CHECKSUM_UNNECESSARY;
> @@ -2105,7 +2114,7 @@ static int xmit_skb(struct send_queue *sq, struct
sk_buff *skb)
>         if (can_push)
>                 hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data -
hdr_len);
>         else
> -               hdr = skb_vnet_hdr(skb);
> +               hdr = &skb_vnet_common_hdr(skb)->mrg_hdr;
>
>         if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
>                                     virtio_is_little_endian(vi->vdev),
false,
> --
> 2.37.1 (Apple Git-137.1)
>

Possibly Parallel Threads

Search for more possibly parallel threads

Virtualization - Aug 2023 - [PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

[PATCH net-next v3] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting

Possibly Parallel Threads