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) >>
Willem de Bruijn
2023-Aug-17 22:11 UTC
[PATCH net-next v2] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting
On Thu, Aug 17, 2023 at 5:52?PM Feng Liu <feliu at 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 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.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 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?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) > >>