| Message ID | 20230817151941.18692-1-feliu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting | expand |
On Thu, Aug 17, 2023 at 11:20 AM Feng Liu <feliu@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@gmail.com> > feedback from Simon Horman <horms@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@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@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) >
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@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@gmail.com> >> feedback from Simon Horman <horms@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@nvidia.com> >> Reviewed-by: Jiri Pirko <jiri@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) >>
On Thu, Aug 17, 2023 at 5:52 PM Feng Liu <feliu@nvidia.com> wrote: > > > > 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@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. I understand in this specific instance. I'm just saying that using sizeof can be wrong both in the new and old case. This does not fix a real bug, as memcpy just uses hdr_len, which is correct. > > 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). Thanks. > > >> > >> Change log > >> v1->v2 > >> feedback from Willem de Bruijn <willemdebruijn.kernel@gmail.com> > >> feedback from Simon Horman <horms@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@nvidia.com> > >> Reviewed-by: Jiri Pirko <jiri@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? That's true. But if we're going to add a helper to bind together alll the virtio variants, then I think it should be there? No strong opinion. Leave out if you prefer and no one else speaks up. > >> @@ -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 we're going to continue to assign to different structs, then I'm honestly not sure how much this patch buys us. Adding virtio_net_hdr to the union also shortens the code btw. Then it can be common_hdr->hdr.flags Also, just a shorter variable name than common_hdr. Fine to call it hdr. > > >> > >> 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) > >>
On 2023-08-17 p.m.6:11, Willem de Bruijn wrote: >>> You mean virtio_net_common_hdr? >>> >> It is a typo, will correct it. >> will do >>> 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. > > I understand in this specific instance. I'm just saying that using sizeof can > be wrong both in the new and old case. > > This does not fix a real bug, as memcpy just uses hdr_len, which is correct. > Already have used hdr_len in the patch instead of sizeof(*hdr) >>> 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). > > Thanks. :-D >> >>>> >>>> Change log >>>> v1->v2 >>>> feedback from Willem de Bruijn <willemdebruijn.kernel@gmail.com> >>>> feedback from Simon Horman <horms@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@nvidia.com> >>>> Reviewed-by: Jiri Pirko <jiri@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? > > That's true. But if we're going to add a helper to bind together alll the > virtio variants, then I think it should be there? > > No strong opinion. Leave out if you prefer and no one else speaks up. > will do >>>> @@ -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 we're going to continue to assign to different structs, then I'm honestly > not sure how much this patch buys us. > > Adding virtio_net_hdr to the union also shortens the code btw. Then it > can be common_hdr->hdr.flags > > Also, just a shorter variable name than common_hdr. Fine to call it hdr. > will do >> >>>> >>>> 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) >>>>
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; + }; +}; + 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; 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,
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. Change log v1->v2 feedback from Willem de Bruijn <willemdebruijn.kernel@gmail.com> feedback from Simon Horman <horms@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@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> --- drivers/net/virtio_net.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-)