Willem de Bruijn
2023-Aug-17 18:26 UTC
[PATCH net-next v2] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting
On Thu, Aug 17, 2023 at 11:20?AM 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 virtio_net_hdr_mrg_rxbuf.You mean virtio_net_common_hdr? I'm not sure I follow the reasoning. Because then hdr_len might be sizeof(virtio_net_hdr_mrg_rxbuf), but sizeof(virtio_net_common_hdr) is larger. So the same issue remains? Indeed, everywhere this patches replaces the one with the other, you have to verify that nothing was using sizeof(*hdr). Which would not be visible from the truncated patch contents itself.> > 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. > > Signed-off-by: Feng Liu <feliu at nvidia.com> > Reviewed-by: Jiri Pirko <jiri at nvidia.com> > --- > drivers/net/virtio_net.c | 36 +++++++++++++++++++++++------------- > 1 file changed, 23 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 1270c8d23463..03cf744de512 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -303,6 +303,13 @@ struct padded_vnet_hdr { > char padding[12]; > }; > > +struct virtio_net_common_hdr { > + union { > + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; > + struct virtio_net_hdr_v1_hash hash_v1_hdr; > + }; > +};Perhaps even add in struct virtio_net_hdr. As that is the original of the three structs, and all the initial fields overlap.> + > 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 +351,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 +477,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 +562,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 +974,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 +1585,8 @@ 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 *common_hdr; > + struct virtio_net_hdr_mrg_rxbuf *mrg_hdr;No more need for this second struct now that we have the union. That's its whole purpose?> > if (unlikely(len < vi->hdr_len + ETH_HLEN)) { > pr_debug("%s: short packet %i\n", dev->name, len); > @@ -1597,18 +1606,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, > if (unlikely(!skb)) > return; > > - hdr = skb_vnet_hdr(skb); > + common_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(&common_hdr->hash_v1_hdr, skb); > > - if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > + mrg_hdr = &common_hdr->mrg_hdr; > + if (mrg_hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) > skb->ip_summed = CHECKSUM_UNNECESSARY; > > - if (virtio_net_hdr_to_skb(skb, &hdr->hdr, > + if (virtio_net_hdr_to_skb(skb, &mrg_hdr->hdr, > virtio_is_little_endian(vi->vdev))) { > net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", > - dev->name, hdr->hdr.gso_type, > - hdr->hdr.gso_size); > + dev->name, mrg_hdr->hdr.gso_type, > + mrg_hdr->hdr.gso_size); > goto frame_err; > } > > @@ -2105,7 +2115,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) >
Feng Liu
2023-Aug-17 21:52 UTC
[PATCH net-next v2] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting
On 2023-08-17 p.m.2:26, Willem de Bruijn wrote:> External email: Use caution opening links or attachments > > > On Thu, Aug 17, 2023 at 11:20?AM 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 virtio_net_hdr_mrg_rxbuf. > > You mean virtio_net_common_hdr? >It is a typo, will correct it.> I'm not sure I follow the reasoning. Because then hdr_len might be > sizeof(virtio_net_hdr_mrg_rxbuf), but sizeof(virtio_net_common_hdr) is > larger. So the same issue remains? >static int virtnet_probe(struct virtio_device *vdev) { [...] if (vi->has_rss_hash_report) { vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash); /* hdr_len will be 20 bytes */ } else if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) || virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) vi->hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf); else vi->hdr_len = sizeof(struct virtio_net_hdr); [...] } When VIRTIO_NET_F_HASH_REPORT is enabled, hdr_len = 20 (as above); and the size of virtio_net_hdr_mrg_rxbuf is 12, so virtio_net_hdr_mrg_rxbuf is wrong, should use struct virtio_net_common_hdr here.> Indeed, everywhere this patches replaces the one with the other, you > have to verify that nothing was using sizeof(*hdr). Which would not be > visible from the truncated patch contents itself. >Have checked. Nothing is using sizeof(*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. >> >> Signed-off-by: Feng Liu <feliu at nvidia.com> >> Reviewed-by: Jiri Pirko <jiri at nvidia.com> >> --- >> drivers/net/virtio_net.c | 36 +++++++++++++++++++++++------------- >> 1 file changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 1270c8d23463..03cf744de512 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -303,6 +303,13 @@ struct padded_vnet_hdr { >> char padding[12]; >> }; >> >> +struct virtio_net_common_hdr { >> + union { >> + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; >> + struct virtio_net_hdr_v1_hash hash_v1_hdr; >> + }; >> +}; > > Perhaps even add in struct virtio_net_hdr. As that is the original of > the three structs, and all the initial fields overlap. >But I didn't use virtio_net_hdr in this patch, is it redundant to put it here? what do you think?>> + >> 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 +351,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 +477,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 +562,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 +974,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 +1585,8 @@ 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 *common_hdr; >> + struct virtio_net_hdr_mrg_rxbuf *mrg_hdr; > > No more need for this second struct now that we have the union. That's > its whole purpose?Yes, struct virtio_net_hdr_mrg_rxbuf *mrg_hdr is not needed. Writing mrg_hdr here is just to make the code look more concise, such as mrg_hdr->hdr.flags, if mrg_hdr is not used, it should be written as common_hdr->mrg_hdr.hdr.flags, I think it looks too long. what you think?>> >> if (unlikely(len < vi->hdr_len + ETH_HLEN)) { >> pr_debug("%s: short packet %i\n", dev->name, len); >> @@ -1597,18 +1606,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq, >> if (unlikely(!skb)) >> return; >> >> - hdr = skb_vnet_hdr(skb); >> + common_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(&common_hdr->hash_v1_hdr, skb); >> >> - if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) >> + mrg_hdr = &common_hdr->mrg_hdr; >> + if (mrg_hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) >> skb->ip_summed = CHECKSUM_UNNECESSARY; >> >> - if (virtio_net_hdr_to_skb(skb, &hdr->hdr, >> + if (virtio_net_hdr_to_skb(skb, &mrg_hdr->hdr, >> virtio_is_little_endian(vi->vdev))) { >> net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n", >> - dev->name, hdr->hdr.gso_type, >> - hdr->hdr.gso_size); >> + dev->name, mrg_hdr->hdr.gso_type, >> + mrg_hdr->hdr.gso_size); >> goto frame_err; >> } >> >> @@ -2105,7 +2115,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) >>