| Message ID | 20230814171845.65930-1-feliu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] virtio_net: Introduce skb_vnet_common_hdr to avoid typecasting | expand |
On Mon, Aug 14, 2023 at 01:18:45PM -0400, Feng Liu wrote: + "David S. Miller" <davem@davemloft.net> Eric Dumazet <edumazet@google.com> Jakub Kicinski <kuba@kernel.org> Paolo Abeni <pabeni@redhat.com> > 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. > > Signed-off-by: Feng Liu <feliu@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@nvidia.com> I'm unsure if this is 'net' material. > --- > drivers/net/virtio_net.c | 29 ++++++++++++++++------------- > include/uapi/linux/virtio_net.h | 7 +++++++ > 2 files changed, 23 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 1270c8d23463..6ce0fbcabda9 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -344,9 +344,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 +470,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 +555,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 +967,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 +1578,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 +1599,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 +2108,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, > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > index 12c1c9699935..db40f93ae8b3 100644 > --- a/include/uapi/linux/virtio_net.h > +++ b/include/uapi/linux/virtio_net.h > @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf { > struct virtio_net_hdr hdr; > __virtio16 num_buffers; /* Number of merged rx buffers */ > }; > + > +struct virtio_net_common_hdr { > + union { > + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; > + struct virtio_net_hdr_v1_hash hash_v1_hdr; > + }; > +}; Does this belong in the UAPI? I would have assumed it's a Kernel implementation detail. > #endif /* ...VIRTIO_NET_NO_LEGACY */ > > /* > -- > 2.37.1 (Apple Git-137.1) > >
On 2023-08-15 a.m.6:51, Simon Horman wrote: > External email: Use caution opening links or attachments > > > On Mon, Aug 14, 2023 at 01:18:45PM -0400, Feng Liu wrote: > > + "David S. Miller" <davem@davemloft.net> > Eric Dumazet <edumazet@google.com> > Jakub Kicinski <kuba@kernel.org> > Paolo Abeni <pabeni@redhat.com> > Thanks for adding David S. Miller. >> 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. >> >> Signed-off-by: Feng Liu <feliu@nvidia.com> >> Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > I'm unsure if this is 'net' material. > It is about the modification of the virtio_net driver. I think it should be regarded as `net` material. >> --- >> drivers/net/virtio_net.c | 29 ++++++++++++++++------------- >> include/uapi/linux/virtio_net.h | 7 +++++++ >> 2 files changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 1270c8d23463..6ce0fbcabda9 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -344,9 +344,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 +470,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 +555,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 +967,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 +1578,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 +1599,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 +2108,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, >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h >> index 12c1c9699935..db40f93ae8b3 100644 >> --- a/include/uapi/linux/virtio_net.h >> +++ b/include/uapi/linux/virtio_net.h >> @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf { >> struct virtio_net_hdr hdr; >> __virtio16 num_buffers; /* Number of merged rx buffers */ >> }; >> + >> +struct virtio_net_common_hdr { >> + union { >> + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; >> + struct virtio_net_hdr_v1_hash hash_v1_hdr; >> + }; >> +}; > > Does this belong in the UAPI? > I would have assumed it's a Kernel implementation detail. > The existing codes, virtio_net.h is in uapi/linux/, I added the new structure and followed existing code. My modification is related to Kernel implementation detail now. >> #endif /* ...VIRTIO_NET_NO_LEGACY */ >> >> /* >> -- >> 2.37.1 (Apple Git-137.1) >> >>
On Tue, Aug 15, 2023 at 11:09:02AM -0400, Feng Liu wrote: > > > On 2023-08-15 a.m.6:51, Simon Horman wrote: > > External email: Use caution opening links or attachments > > > > > > On Mon, Aug 14, 2023 at 01:18:45PM -0400, Feng Liu wrote: > > > > + "David S. Miller" <davem@davemloft.net> > > Eric Dumazet <edumazet@google.com> > > Jakub Kicinski <kuba@kernel.org> > > Paolo Abeni <pabeni@redhat.com> > > > Thanks for adding David S. Miller. > > > > 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. > > > > > > Signed-off-by: Feng Liu <feliu@nvidia.com> > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > > > I'm unsure if this is 'net' material. > > > > It is about the modification of the virtio_net driver. I think it should be > regarded as `net` material. To clarify: In general new Networking features go via the net-next tree, while bug fixes go via the net tree. I was suggesting this is more appropriate for net-next, and that should be reflected in the subject. Subject: [PATCH net-next] ... Sorry for not being clearer the first time around. > > > > --- > > > drivers/net/virtio_net.c | 29 ++++++++++++++++------------- > > > include/uapi/linux/virtio_net.h | 7 +++++++ > > > 2 files changed, 23 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 1270c8d23463..6ce0fbcabda9 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -344,9 +344,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 +470,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 +555,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 +967,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 +1578,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 +1599,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 +2108,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, > > > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > > > index 12c1c9699935..db40f93ae8b3 100644 > > > --- a/include/uapi/linux/virtio_net.h > > > +++ b/include/uapi/linux/virtio_net.h > > > @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf { > > > struct virtio_net_hdr hdr; > > > __virtio16 num_buffers; /* Number of merged rx buffers */ > > > }; > > > + > > > +struct virtio_net_common_hdr { > > > + union { > > > + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; > > > + struct virtio_net_hdr_v1_hash hash_v1_hdr; > > > + }; > > > +}; > > > > Does this belong in the UAPI? > > I would have assumed it's a Kernel implementation detail. > > > The existing codes, virtio_net.h is in uapi/linux/, I added the new > structure and followed existing code. My modification is related to Kernel > implementation detail now. The header you have modified forms part of the userspace API (UAPI). Perhaps there is something about virtio_net that makes this correct, but it seems to me that kernel-internal details don't belong there. > > > > #endif /* ...VIRTIO_NET_NO_LEGACY */ > > > > > > /* > > > -- > > > 2.37.1 (Apple Git-137.1) > > > > > > >
On Tue, Aug 15, 2023 at 12:29 PM Simon Horman <horms@kernel.org> wrote: > > On Tue, Aug 15, 2023 at 11:09:02AM -0400, Feng Liu wrote: > > > > > > On 2023-08-15 a.m.6:51, Simon Horman wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On Mon, Aug 14, 2023 at 01:18:45PM -0400, Feng Liu wrote: > > > > > > + "David S. Miller" <davem@davemloft.net> > > > Eric Dumazet <edumazet@google.com> > > > Jakub Kicinski <kuba@kernel.org> > > > Paolo Abeni <pabeni@redhat.com> > > > > > Thanks for adding David S. Miller. > > > > > > 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. > > > > > > > > Signed-off-by: Feng Liu <feliu@nvidia.com> > > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > > > > > I'm unsure if this is 'net' material. > > > > > > > It is about the modification of the virtio_net driver. I think it should be > > regarded as `net` material. > > To clarify: In general new Networking features go via the net-next tree, > while bug fixes go via the net tree. I was suggesting this > is more appropriate for net-next, and that should be reflected in the > subject. > > Subject: [PATCH net-next] ... > > Sorry for not being clearer the first time around. Right, this should go to net-next. > > > > > > > --- > > > > drivers/net/virtio_net.c | 29 ++++++++++++++++------------- > > > > include/uapi/linux/virtio_net.h | 7 +++++++ > > > > 2 files changed, 23 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index 1270c8d23463..6ce0fbcabda9 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -344,9 +344,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 +470,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 +555,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 +967,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 +1578,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 +1599,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 +2108,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, > > > > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > > > > index 12c1c9699935..db40f93ae8b3 100644 > > > > --- a/include/uapi/linux/virtio_net.h > > > > +++ b/include/uapi/linux/virtio_net.h > > > > @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf { > > > > struct virtio_net_hdr hdr; > > > > __virtio16 num_buffers; /* Number of merged rx buffers */ > > > > }; > > > > + > > > > +struct virtio_net_common_hdr { > > > > + union { > > > > + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; > > > > + struct virtio_net_hdr_v1_hash hash_v1_hdr; > > > > + }; > > > > +}; > > > > > > Does this belong in the UAPI? > > > I would have assumed it's a Kernel implementation detail. > > > > > The existing codes, virtio_net.h is in uapi/linux/, I added the new > > structure and followed existing code. My modification is related to Kernel > > implementation detail now. > > The header you have modified forms part of the userspace API (UAPI). > Perhaps there is something about virtio_net that makes this correct, but it > seems to me that kernel-internal details don't belong there. FWIW, I ran into similar issues before in a draft that added timestamp support [1] If we're going to change this structure, we should do it in a way that is forward proof to future extensions to the virtio spec and with that the fields in this struct. Especially in UAPI. Is virtio_net_hdr_v1_hash the latest virtio-spec compliant header? And do we expect for v1.3 to just add some fields to this? The struct comment of virtio_net_hdr_v1 states "This is bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf, only flattened.". I don't quite understand what the flattening bought, vs having struct virtio_net_hdr as first member. Another difference may be the endianness between legacy (0.9) and v1.0+. Since legacy virtio will no longer be modified, I don't think there is much value is exposing this new union as UAPI. I do appreciate the benefit to the implementation. [1] https://patches.linaro.org/project/netdev/patch/20210208185558.995292-3-willemdebruijn.kernel@gmail.com/
On 2023-08-15 p.m.2:13, Willem de Bruijn wrote: > External email: Use caution opening links or attachments > > > On Tue, Aug 15, 2023 at 12:29 PM Simon Horman <horms@kernel.org> wrote: >> >> On Tue, Aug 15, 2023 at 11:09:02AM -0400, Feng Liu wrote: >> To clarify: In general new Networking features go via the net-next tree, >> while bug fixes go via the net tree. I was suggesting this >> is more appropriate for net-next, and that should be reflected in the >> subject. >> >> Subject: [PATCH net-next] ... >> >> Sorry for not being clearer the first time around. > > Right, this should go to net-next. > Will do, thanks >> >>> >>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h >>>>> index 12c1c9699935..db40f93ae8b3 100644 >>>>> --- a/include/uapi/linux/virtio_net.h >>>>> +++ b/include/uapi/linux/virtio_net.h >>>>> @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf { >>>>> struct virtio_net_hdr hdr; >>>>> __virtio16 num_buffers; /* Number of merged rx buffers */ >>>>> }; >>>>> + >>>>> +struct virtio_net_common_hdr { >>>>> + union { >>>>> + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; >>>>> + struct virtio_net_hdr_v1_hash hash_v1_hdr; >>>>> + }; >>>>> +}; >>>> >>>> Does this belong in the UAPI? >>>> I would have assumed it's a Kernel implementation detail. >>>> >>> The existing codes, virtio_net.h is in uapi/linux/, I added the new >>> structure and followed existing code. My modification is related to Kernel >>> implementation detail now. >> >> The header you have modified forms part of the userspace API (UAPI). >> Perhaps there is something about virtio_net that makes this correct, but it >> seems to me that kernel-internal details don't belong there. > > FWIW, I ran into similar issues before in a draft that added timestamp > support [1] > > If we're going to change this structure, we should do it in a way that > is forward proof to future extensions to the virtio spec and with that > the fields in this struct. Especially in UAPI. > > Is virtio_net_hdr_v1_hash the latest virtio-spec compliant header? And > do we expect for v1.3 to just add some fields to this? > > The struct comment of virtio_net_hdr_v1 states "This is > bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf, only > flattened.". I don't quite understand what the flattening bought, vs > having struct virtio_net_hdr as first member. Another difference may > be the endianness between legacy (0.9) and v1.0+. > > Since legacy virtio will no longer be modified, I don't think there is > much value is exposing this new union as UAPI. I do appreciate the > benefit to the implementation. > > [1] https://patches.linaro.org/project/netdev/patch/20210208185558.995292-3-willemdebruijn.kernel@gmail.com/ Hi, William and Simon Thanks for the detailed explanation. I kept virtio_net_hdr_mrg_rxbuf and virtio_net_hdr_v1_hash structures in virtio_net.h, which can be forward compatible with existing user applications which use these structures. After checking kernel code, the virtio_net_hdr_v1_hash structure does only add new members to virtio_net_hdr_mrg_rxbuf, so the spec should only add new members, otherwise there will be compatibility problems in struct virtio_net_hdr_v1_hash structure. struct virtio_net_hdr_v1_hash { struct virtio_net_hdr_v1 hdr; /*same size as virtio_net_hdr*/ [...] __le32 hash_value; /*new member*/ __le16 hash_report; /*new member*/ __le16 padding; /*new member*/ }; virtio_net_hdr_v1_hash cannot use virtio_net_hdr as the first member, because in virtio_net_hdr_v1, csum_start and csum_offset are stored in union as a structure, and virtio_net_hdr cannot be used instead. struct virtio_net_hdr_v1 { [...] union { struct { __virtio16 csum_start; __virtio16 csum_offset; }; [...] }; __virtio16 num_buffers; /* Number of merged rx buffers */ }; struct virtio_net_hdr { [...] __virtio16 csum_start; __virtio16 csum_offset; }; In addition, I put this new structure virtio_net_common_hdr in uapi, hoping it could be used in future user space application to avoid potential risks caused by type coercion (such as the problems mentioned in the patch description ). So I think it should be in this header file. What do you think?
> > > > Since legacy virtio will no longer be modified, I don't think there is > > much value is exposing this new union as UAPI. I do appreciate the > > benefit to the implementation. > > > > [1] https://patches.linaro.org/project/netdev/patch/20210208185558.995292-3-willemdebruijn.kernel@gmail.com/ > Hi, William and Simon > > Thanks for the detailed explanation. > > I kept virtio_net_hdr_mrg_rxbuf and virtio_net_hdr_v1_hash structures in > virtio_net.h, which can be forward compatible with existing user > applications which use these structures. They're UAPI, so we cannot modify or remove them anyway. Which is exactly why we want to be careful with adding anything new. > virtio_net_hdr_v1_hash cannot use virtio_net_hdr as the first member, > because in virtio_net_hdr_v1, csum_start and csum_offset are stored in > union as a structure, and virtio_net_hdr cannot be used instead. Oh right. That wasn't always the case, or the reason for this. Not super relevant but, commit ed9ecb0415b9 has the history virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined. In particular, the virtio header always has the u16 num_buffers field. We define a new 'struct virtio_net_hdr_v1' for this (rather than simply calling it 'struct virtio_net_hdr', to avoid nasty type errors if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't. Transitional devices (which can't define VIRTIO_NET_NO_LEGACY) will have to keep using struct virtio_net_hdr_mrg_rxbuf, which has the same byte layout as struct virtio_net_hdr_v1. The union was added to overload csum use on tx with RSC use on rx, in commit 22b436c9b568. I don't quite follow why there now are three structs, rather than two. The first two seem to both implement csum partial. Anyway, not super important here. > In addition, I put this new structure virtio_net_common_hdr in uapi, > hoping it could be used in future user space application to avoid > potential risks caused by type coercion (such as the problems mentioned > in the patch description ). So I think it should be in this header file. > What do you think? Adding anything to UAPI has a high bar. Do you have a concrete use case for this? This does seem mostly a helper to simplify kernel logic to me, which is better kept in non-UAPI headers.
On 2023-08-16 a.m.10:53, Willem de Bruijn wrote: > External email: Use caution opening links or attachments > > >> >> Thanks for the detailed explanation. >> >> I kept virtio_net_hdr_mrg_rxbuf and virtio_net_hdr_v1_hash structures in >> virtio_net.h, which can be forward compatible with existing user >> applications which use these structures. > > They're UAPI, so we cannot modify or remove them anyway. > > Which is exactly why we want to be careful with adding anything new. > ok >> virtio_net_hdr_v1_hash cannot use virtio_net_hdr as the first member, >> because in virtio_net_hdr_v1, csum_start and csum_offset are stored in >> union as a structure, and virtio_net_hdr cannot be used instead. > > Oh right. That wasn't always the case, or the reason for this. > Not super relevant but, commit ed9ecb0415b9 has the history > > virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined. > > In particular, the virtio header always has the u16 num_buffers field. > We define a new 'struct virtio_net_hdr_v1' for this (rather than > simply calling it 'struct virtio_net_hdr', to avoid nasty type errors > if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't. > > Transitional devices (which can't define VIRTIO_NET_NO_LEGACY) will > have to keep using struct virtio_net_hdr_mrg_rxbuf, which has the same > byte layout as struct virtio_net_hdr_v1. > > The union was added to overload csum use on tx with RSC use on rx, in > commit 22b436c9b568. I don't quite follow why there now are three > structs, rather than two. The first two seem to both implement csum > partial. Anyway, not super important here. >ok >> In addition, I put this new structure virtio_net_common_hdr in uapi, >> hoping it could be used in future user space application to avoid >> potential risks caused by type coercion (such as the problems mentioned >> in the patch description ). So I think it should be in this header file. >> What do you think? > > Adding anything to UAPI has a high bar. Do you have a concrete use > case for this? In the scene of with and without VIRTIO_NET_F_HASH_REPORT feature, this patch has been tested on my setup, and the function is ok. > > This does seem mostly a helper to simplify kernel logic to me, which > is better kept in non-UAPI headers. OK, will change it.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 1270c8d23463..6ce0fbcabda9 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -344,9 +344,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 +470,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 +555,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 +967,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 +1578,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 +1599,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 +2108,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, diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h index 12c1c9699935..db40f93ae8b3 100644 --- a/include/uapi/linux/virtio_net.h +++ b/include/uapi/linux/virtio_net.h @@ -201,6 +201,13 @@ struct virtio_net_hdr_mrg_rxbuf { struct virtio_net_hdr hdr; __virtio16 num_buffers; /* Number of merged rx buffers */ }; + +struct virtio_net_common_hdr { + union { + struct virtio_net_hdr_mrg_rxbuf mrg_hdr; + struct virtio_net_hdr_v1_hash hash_v1_hdr; + }; +}; #endif /* ...VIRTIO_NET_NO_LEGACY */ /*
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. Signed-off-by: Feng Liu <feliu@nvidia.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com> --- drivers/net/virtio_net.c | 29 ++++++++++++++++------------- include/uapi/linux/virtio_net.h | 7 +++++++ 2 files changed, 23 insertions(+), 13 deletions(-)