| Message ID | 20250109-tun-v2-3-388d7d5a287a@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tun: Unify vnet implementation and fill full vnet header | expand |
On Thu, Jan 09, 2025 at 03:58:45PM +0900, Akihiko Odaki wrote: > The specification says the device MUST set num_buffers to 1 if > VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> How do we know this is v1 and not v0? Confused. > --- > drivers/net/tap.c | 2 +- > drivers/net/tun.c | 6 ++++-- > drivers/net/tun_vnet.c | 14 +++++++++----- > drivers/net/tun_vnet.h | 4 ++-- > 4 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/tap.c b/drivers/net/tap.c > index 60804855510b..fe9554ee5b8b 100644 > --- a/drivers/net/tap.c > +++ b/drivers/net/tap.c > @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, > int total; > > if (q->flags & IFF_VNET_HDR) { > - struct virtio_net_hdr vnet_hdr; > + struct virtio_net_hdr_v1 vnet_hdr; > > vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index dbf0dee92e93..f211d0580887 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, > size_t total; > > if (tun->flags & IFF_VNET_HDR) { > - struct virtio_net_hdr gso = { 0 }; > + struct virtio_net_hdr_v1 gso = { > + .num_buffers = __virtio16_to_cpu(true, 1) > + }; > > vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); > ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); > @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, > } > > if (vnet_hdr_sz) { > - struct virtio_net_hdr gso; > + struct virtio_net_hdr_v1 gso; > > ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); > if (ret < 0) > diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c > index ffb2186facd3..a7a7989fae56 100644 > --- a/drivers/net/tun_vnet.c > +++ b/drivers/net/tun_vnet.c > @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, > EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); > > int tun_vnet_hdr_put(int sz, struct iov_iter *iter, > - const struct virtio_net_hdr *hdr) > + const struct virtio_net_hdr_v1 *hdr) > { > + int content_sz = MIN(sizeof(*hdr), sz); > + > if (iov_iter_count(iter) < sz) > return -EINVAL; > > - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) > + if (copy_to_iter(hdr, content_sz, iter) != content_sz) > return -EFAULT; > > - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) > + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) > return -EFAULT; > > return 0; > @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); > > int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > const struct sk_buff *skb, > - struct virtio_net_hdr *hdr) > + struct virtio_net_hdr_v1 *hdr) > { > int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; > > - if (virtio_net_hdr_from_skb(skb, hdr, > + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, > tun_vnet_is_little_endian(flags), true, > vlan_hlen)) { > struct skb_shared_info *sinfo = skb_shinfo(skb); > @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > return -EINVAL; > } > > + hdr->num_buffers = 1; > + > return 0; > } > EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > index 2dfdbe92bb24..d8fd94094227 100644 > --- a/drivers/net/tun_vnet.h > +++ b/drivers/net/tun_vnet.h > @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, > struct virtio_net_hdr *hdr); > > int tun_vnet_hdr_put(int sz, struct iov_iter *iter, > - const struct virtio_net_hdr *hdr); > + const struct virtio_net_hdr_v1 *hdr); > > int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, > const struct virtio_net_hdr *hdr); > > int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > const struct sk_buff *skb, > - struct virtio_net_hdr *hdr); > + struct virtio_net_hdr_v1 *hdr); > > #endif /* TUN_VNET_H */ > > -- > 2.47.1
On Thu, Jan 09, 2025 at 02:32:25AM -0500, Michael S. Tsirkin wrote: > On Thu, Jan 09, 2025 at 03:58:45PM +0900, Akihiko Odaki wrote: > > The specification says the device MUST set num_buffers to 1 if > > VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > How do we know this is v1 and not v0? Confused. Ah I got it, you assume userspace will over-write it if VIRTIO_NET_F_MRG_RXBUF is set. If we are leaving this up to userspace, why not let it do it always? > > --- > > drivers/net/tap.c | 2 +- > > drivers/net/tun.c | 6 ++++-- > > drivers/net/tun_vnet.c | 14 +++++++++----- > > drivers/net/tun_vnet.h | 4 ++-- > > 4 files changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/tap.c b/drivers/net/tap.c > > index 60804855510b..fe9554ee5b8b 100644 > > --- a/drivers/net/tap.c > > +++ b/drivers/net/tap.c > > @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, > > int total; > > > > if (q->flags & IFF_VNET_HDR) { > > - struct virtio_net_hdr vnet_hdr; > > + struct virtio_net_hdr_v1 vnet_hdr; > > > > vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index dbf0dee92e93..f211d0580887 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, > > size_t total; > > > > if (tun->flags & IFF_VNET_HDR) { > > - struct virtio_net_hdr gso = { 0 }; > > + struct virtio_net_hdr_v1 gso = { > > + .num_buffers = __virtio16_to_cpu(true, 1) > > + }; > > > > vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); > > ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); > > @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, > > } > > > > if (vnet_hdr_sz) { > > - struct virtio_net_hdr gso; > > + struct virtio_net_hdr_v1 gso; > > > > ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); > > if (ret < 0) > > diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c > > index ffb2186facd3..a7a7989fae56 100644 > > --- a/drivers/net/tun_vnet.c > > +++ b/drivers/net/tun_vnet.c > > @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, > > EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); > > > > int tun_vnet_hdr_put(int sz, struct iov_iter *iter, > > - const struct virtio_net_hdr *hdr) > > + const struct virtio_net_hdr_v1 *hdr) > > { > > + int content_sz = MIN(sizeof(*hdr), sz); > > + > > if (iov_iter_count(iter) < sz) > > return -EINVAL; > > > > - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) > > + if (copy_to_iter(hdr, content_sz, iter) != content_sz) > > return -EFAULT; > > > > - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) > > + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) > > return -EFAULT; > > > > return 0; > > @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); > > > > int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > > const struct sk_buff *skb, > > - struct virtio_net_hdr *hdr) > > + struct virtio_net_hdr_v1 *hdr) > > { > > int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; > > > > - if (virtio_net_hdr_from_skb(skb, hdr, > > + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, > > tun_vnet_is_little_endian(flags), true, > > vlan_hlen)) { > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > > return -EINVAL; > > } > > > > + hdr->num_buffers = 1; > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); > > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > > index 2dfdbe92bb24..d8fd94094227 100644 > > --- a/drivers/net/tun_vnet.h > > +++ b/drivers/net/tun_vnet.h > > @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, > > struct virtio_net_hdr *hdr); > > > > int tun_vnet_hdr_put(int sz, struct iov_iter *iter, > > - const struct virtio_net_hdr *hdr); > > + const struct virtio_net_hdr_v1 *hdr); > > > > int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, > > const struct virtio_net_hdr *hdr); > > > > int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > > const struct sk_buff *skb, > > - struct virtio_net_hdr *hdr); > > + struct virtio_net_hdr_v1 *hdr); > > > > #endif /* TUN_VNET_H */ > > > > -- > > 2.47.1
On 2025/01/09 16:40, Michael S. Tsirkin wrote: > On Thu, Jan 09, 2025 at 02:32:25AM -0500, Michael S. Tsirkin wrote: >> On Thu, Jan 09, 2025 at 03:58:45PM +0900, Akihiko Odaki wrote: >>> The specification says the device MUST set num_buffers to 1 if >>> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> >> >> How do we know this is v1 and not v0? Confused. > > Ah I got it, you assume userspace will over-write it > if VIRTIO_NET_F_MRG_RXBUF is set. > If we are leaving this up to userspace, why not let it do > it always? tun may be used with vhost_net, which does not set the field. > >>> --- >>> drivers/net/tap.c | 2 +- >>> drivers/net/tun.c | 6 ++++-- >>> drivers/net/tun_vnet.c | 14 +++++++++----- >>> drivers/net/tun_vnet.h | 4 ++-- >>> 4 files changed, 16 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c >>> index 60804855510b..fe9554ee5b8b 100644 >>> --- a/drivers/net/tap.c >>> +++ b/drivers/net/tap.c >>> @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, >>> int total; >>> >>> if (q->flags & IFF_VNET_HDR) { >>> - struct virtio_net_hdr vnet_hdr; >>> + struct virtio_net_hdr_v1 vnet_hdr; >>> >>> vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); >>> >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >>> index dbf0dee92e93..f211d0580887 100644 >>> --- a/drivers/net/tun.c >>> +++ b/drivers/net/tun.c >>> @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, >>> size_t total; >>> >>> if (tun->flags & IFF_VNET_HDR) { >>> - struct virtio_net_hdr gso = { 0 }; >>> + struct virtio_net_hdr_v1 gso = { >>> + .num_buffers = __virtio16_to_cpu(true, 1) >>> + }; >>> >>> vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); >>> ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); >>> @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, >>> } >>> >>> if (vnet_hdr_sz) { >>> - struct virtio_net_hdr gso; >>> + struct virtio_net_hdr_v1 gso; >>> >>> ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); >>> if (ret < 0) >>> diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c >>> index ffb2186facd3..a7a7989fae56 100644 >>> --- a/drivers/net/tun_vnet.c >>> +++ b/drivers/net/tun_vnet.c >>> @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, >>> EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); >>> >>> int tun_vnet_hdr_put(int sz, struct iov_iter *iter, >>> - const struct virtio_net_hdr *hdr) >>> + const struct virtio_net_hdr_v1 *hdr) >>> { >>> + int content_sz = MIN(sizeof(*hdr), sz); >>> + >>> if (iov_iter_count(iter) < sz) >>> return -EINVAL; >>> >>> - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) >>> + if (copy_to_iter(hdr, content_sz, iter) != content_sz) >>> return -EFAULT; >>> >>> - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) >>> + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) >>> return -EFAULT; >>> >>> return 0; >>> @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); >>> >>> int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, >>> const struct sk_buff *skb, >>> - struct virtio_net_hdr *hdr) >>> + struct virtio_net_hdr_v1 *hdr) >>> { >>> int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; >>> >>> - if (virtio_net_hdr_from_skb(skb, hdr, >>> + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, >>> tun_vnet_is_little_endian(flags), true, >>> vlan_hlen)) { >>> struct skb_shared_info *sinfo = skb_shinfo(skb); >>> @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, >>> return -EINVAL; >>> } >>> >>> + hdr->num_buffers = 1; >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); >>> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h >>> index 2dfdbe92bb24..d8fd94094227 100644 >>> --- a/drivers/net/tun_vnet.h >>> +++ b/drivers/net/tun_vnet.h >>> @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, >>> struct virtio_net_hdr *hdr); >>> >>> int tun_vnet_hdr_put(int sz, struct iov_iter *iter, >>> - const struct virtio_net_hdr *hdr); >>> + const struct virtio_net_hdr_v1 *hdr); >>> >>> int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, >>> const struct virtio_net_hdr *hdr); >>> >>> int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, >>> const struct sk_buff *skb, >>> - struct virtio_net_hdr *hdr); >>> + struct virtio_net_hdr_v1 *hdr); >>> >>> #endif /* TUN_VNET_H */ >>> >>> -- >>> 2.47.1 >
On Thu, Jan 09, 2025 at 06:38:10PM +0900, Akihiko Odaki wrote: > On 2025/01/09 16:40, Michael S. Tsirkin wrote: > > On Thu, Jan 09, 2025 at 02:32:25AM -0500, Michael S. Tsirkin wrote: > > > On Thu, Jan 09, 2025 at 03:58:45PM +0900, Akihiko Odaki wrote: > > > > The specification says the device MUST set num_buffers to 1 if > > > > VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > > > > > How do we know this is v1 and not v0? Confused. > > > > Ah I got it, you assume userspace will over-write it > > if VIRTIO_NET_F_MRG_RXBUF is set. > > If we are leaving this up to userspace, why not let it do > > it always? > > tun may be used with vhost_net, which does not set the field. I'd fix that in vhost net. > > > > > > --- > > > > drivers/net/tap.c | 2 +- > > > > drivers/net/tun.c | 6 ++++-- > > > > drivers/net/tun_vnet.c | 14 +++++++++----- > > > > drivers/net/tun_vnet.h | 4 ++-- > > > > 4 files changed, 16 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/net/tap.c b/drivers/net/tap.c > > > > index 60804855510b..fe9554ee5b8b 100644 > > > > --- a/drivers/net/tap.c > > > > +++ b/drivers/net/tap.c > > > > @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, > > > > int total; > > > > if (q->flags & IFF_VNET_HDR) { > > > > - struct virtio_net_hdr vnet_hdr; > > > > + struct virtio_net_hdr_v1 vnet_hdr; > > > > vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > > index dbf0dee92e93..f211d0580887 100644 > > > > --- a/drivers/net/tun.c > > > > +++ b/drivers/net/tun.c > > > > @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, > > > > size_t total; > > > > if (tun->flags & IFF_VNET_HDR) { > > > > - struct virtio_net_hdr gso = { 0 }; > > > > + struct virtio_net_hdr_v1 gso = { > > > > + .num_buffers = __virtio16_to_cpu(true, 1) > > > > + }; > > > > vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); > > > > ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); > > > > @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, > > > > } > > > > if (vnet_hdr_sz) { > > > > - struct virtio_net_hdr gso; > > > > + struct virtio_net_hdr_v1 gso; > > > > ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); > > > > if (ret < 0) > > > > diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c > > > > index ffb2186facd3..a7a7989fae56 100644 > > > > --- a/drivers/net/tun_vnet.c > > > > +++ b/drivers/net/tun_vnet.c > > > > @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, > > > > EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); > > > > int tun_vnet_hdr_put(int sz, struct iov_iter *iter, > > > > - const struct virtio_net_hdr *hdr) > > > > + const struct virtio_net_hdr_v1 *hdr) > > > > { > > > > + int content_sz = MIN(sizeof(*hdr), sz); > > > > + > > > > if (iov_iter_count(iter) < sz) > > > > return -EINVAL; > > > > - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) > > > > + if (copy_to_iter(hdr, content_sz, iter) != content_sz) > > > > return -EFAULT; > > > > - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) > > > > + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) > > > > return -EFAULT; > > > > return 0; > > > > @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); > > > > int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > > > > const struct sk_buff *skb, > > > > - struct virtio_net_hdr *hdr) > > > > + struct virtio_net_hdr_v1 *hdr) > > > > { > > > > int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; > > > > - if (virtio_net_hdr_from_skb(skb, hdr, > > > > + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, > > > > tun_vnet_is_little_endian(flags), true, > > > > vlan_hlen)) { > > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > > > > return -EINVAL; > > > > } > > > > + hdr->num_buffers = 1; > > > > + > > > > return 0; > > > > } > > > > EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); > > > > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > > > > index 2dfdbe92bb24..d8fd94094227 100644 > > > > --- a/drivers/net/tun_vnet.h > > > > +++ b/drivers/net/tun_vnet.h > > > > @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, > > > > struct virtio_net_hdr *hdr); > > > > int tun_vnet_hdr_put(int sz, struct iov_iter *iter, > > > > - const struct virtio_net_hdr *hdr); > > > > + const struct virtio_net_hdr_v1 *hdr); > > > > int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, > > > > const struct virtio_net_hdr *hdr); > > > > int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, > > > > const struct sk_buff *skb, > > > > - struct virtio_net_hdr *hdr); > > > > + struct virtio_net_hdr_v1 *hdr); > > > > #endif /* TUN_VNET_H */ > > > > > > > > -- > > > > 2.47.1 > >
On 2025/01/09 19:54, Michael S. Tsirkin wrote: > On Thu, Jan 09, 2025 at 06:38:10PM +0900, Akihiko Odaki wrote: >> On 2025/01/09 16:40, Michael S. Tsirkin wrote: >>> On Thu, Jan 09, 2025 at 02:32:25AM -0500, Michael S. Tsirkin wrote: >>>> On Thu, Jan 09, 2025 at 03:58:45PM +0900, Akihiko Odaki wrote: >>>>> The specification says the device MUST set num_buffers to 1 if >>>>> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. >>>>> >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> >>>> >>>> How do we know this is v1 and not v0? Confused. >>> >>> Ah I got it, you assume userspace will over-write it >>> if VIRTIO_NET_F_MRG_RXBUF is set. >>> If we are leaving this up to userspace, why not let it do >>> it always? >> >> tun may be used with vhost_net, which does not set the field. > > I'd fix that in vhost net. Let's see what filesystem and networking people will say for the earlier patch. We can fix num_buffers for free if the earlier patch is getting merged. We will need to come up with another solution otherwise. > > >>> >>>>> --- >>>>> drivers/net/tap.c | 2 +- >>>>> drivers/net/tun.c | 6 ++++-- >>>>> drivers/net/tun_vnet.c | 14 +++++++++----- >>>>> drivers/net/tun_vnet.h | 4 ++-- >>>>> 4 files changed, 16 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c >>>>> index 60804855510b..fe9554ee5b8b 100644 >>>>> --- a/drivers/net/tap.c >>>>> +++ b/drivers/net/tap.c >>>>> @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, >>>>> int total; >>>>> if (q->flags & IFF_VNET_HDR) { >>>>> - struct virtio_net_hdr vnet_hdr; >>>>> + struct virtio_net_hdr_v1 vnet_hdr; >>>>> vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); >>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >>>>> index dbf0dee92e93..f211d0580887 100644 >>>>> --- a/drivers/net/tun.c >>>>> +++ b/drivers/net/tun.c >>>>> @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, >>>>> size_t total; >>>>> if (tun->flags & IFF_VNET_HDR) { >>>>> - struct virtio_net_hdr gso = { 0 }; >>>>> + struct virtio_net_hdr_v1 gso = { >>>>> + .num_buffers = __virtio16_to_cpu(true, 1) >>>>> + }; >>>>> vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); >>>>> ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); >>>>> @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, >>>>> } >>>>> if (vnet_hdr_sz) { >>>>> - struct virtio_net_hdr gso; >>>>> + struct virtio_net_hdr_v1 gso; >>>>> ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); >>>>> if (ret < 0) >>>>> diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c >>>>> index ffb2186facd3..a7a7989fae56 100644 >>>>> --- a/drivers/net/tun_vnet.c >>>>> +++ b/drivers/net/tun_vnet.c >>>>> @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, >>>>> EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); >>>>> int tun_vnet_hdr_put(int sz, struct iov_iter *iter, >>>>> - const struct virtio_net_hdr *hdr) >>>>> + const struct virtio_net_hdr_v1 *hdr) >>>>> { >>>>> + int content_sz = MIN(sizeof(*hdr), sz); >>>>> + >>>>> if (iov_iter_count(iter) < sz) >>>>> return -EINVAL; >>>>> - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) >>>>> + if (copy_to_iter(hdr, content_sz, iter) != content_sz) >>>>> return -EFAULT; >>>>> - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) >>>>> + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) >>>>> return -EFAULT; >>>>> return 0; >>>>> @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); >>>>> int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, >>>>> const struct sk_buff *skb, >>>>> - struct virtio_net_hdr *hdr) >>>>> + struct virtio_net_hdr_v1 *hdr) >>>>> { >>>>> int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; >>>>> - if (virtio_net_hdr_from_skb(skb, hdr, >>>>> + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, >>>>> tun_vnet_is_little_endian(flags), true, >>>>> vlan_hlen)) { >>>>> struct skb_shared_info *sinfo = skb_shinfo(skb); >>>>> @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, >>>>> return -EINVAL; >>>>> } >>>>> + hdr->num_buffers = 1; >>>>> + >>>>> return 0; >>>>> } >>>>> EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); >>>>> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h >>>>> index 2dfdbe92bb24..d8fd94094227 100644 >>>>> --- a/drivers/net/tun_vnet.h >>>>> +++ b/drivers/net/tun_vnet.h >>>>> @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, >>>>> struct virtio_net_hdr *hdr); >>>>> int tun_vnet_hdr_put(int sz, struct iov_iter *iter, >>>>> - const struct virtio_net_hdr *hdr); >>>>> + const struct virtio_net_hdr_v1 *hdr); >>>>> int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, >>>>> const struct virtio_net_hdr *hdr); >>>>> int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, >>>>> const struct sk_buff *skb, >>>>> - struct virtio_net_hdr *hdr); >>>>> + struct virtio_net_hdr_v1 *hdr); >>>>> #endif /* TUN_VNET_H */ >>>>> >>>>> -- >>>>> 2.47.1 >>> >
diff --git a/drivers/net/tap.c b/drivers/net/tap.c index 60804855510b..fe9554ee5b8b 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -713,7 +713,7 @@ static ssize_t tap_put_user(struct tap_queue *q, int total; if (q->flags & IFF_VNET_HDR) { - struct virtio_net_hdr vnet_hdr; + struct virtio_net_hdr_v1 vnet_hdr; vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); diff --git a/drivers/net/tun.c b/drivers/net/tun.c index dbf0dee92e93..f211d0580887 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1991,7 +1991,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun, size_t total; if (tun->flags & IFF_VNET_HDR) { - struct virtio_net_hdr gso = { 0 }; + struct virtio_net_hdr_v1 gso = { + .num_buffers = __virtio16_to_cpu(true, 1) + }; vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz); ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso); @@ -2044,7 +2046,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, } if (vnet_hdr_sz) { - struct virtio_net_hdr gso; + struct virtio_net_hdr_v1 gso; ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso); if (ret < 0) diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c index ffb2186facd3..a7a7989fae56 100644 --- a/drivers/net/tun_vnet.c +++ b/drivers/net/tun_vnet.c @@ -130,15 +130,17 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); int tun_vnet_hdr_put(int sz, struct iov_iter *iter, - const struct virtio_net_hdr *hdr) + const struct virtio_net_hdr_v1 *hdr) { + int content_sz = MIN(sizeof(*hdr), sz); + if (iov_iter_count(iter) < sz) return -EINVAL; - if (copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)) + if (copy_to_iter(hdr, content_sz, iter) != content_sz) return -EFAULT; - if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) + if (iov_iter_zero(sz - content_sz, iter) != sz - content_sz) return -EFAULT; return 0; @@ -154,11 +156,11 @@ EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, const struct sk_buff *skb, - struct virtio_net_hdr *hdr) + struct virtio_net_hdr_v1 *hdr) { int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; - if (virtio_net_hdr_from_skb(skb, hdr, + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr, tun_vnet_is_little_endian(flags), true, vlan_hlen)) { struct skb_shared_info *sinfo = skb_shinfo(skb); @@ -176,6 +178,8 @@ int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, return -EINVAL; } + hdr->num_buffers = 1; + return 0; } EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb); diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h index 2dfdbe92bb24..d8fd94094227 100644 --- a/drivers/net/tun_vnet.h +++ b/drivers/net/tun_vnet.h @@ -12,13 +12,13 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, struct virtio_net_hdr *hdr); int tun_vnet_hdr_put(int sz, struct iov_iter *iter, - const struct virtio_net_hdr *hdr); + const struct virtio_net_hdr_v1 *hdr); int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, const struct virtio_net_hdr *hdr); int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, const struct sk_buff *skb, - struct virtio_net_hdr *hdr); + struct virtio_net_hdr_v1 *hdr); #endif /* TUN_VNET_H */
The specification says the device MUST set num_buffers to 1 if VIRTIO_NET_F_MRG_RXBUF has not been negotiated. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- drivers/net/tap.c | 2 +- drivers/net/tun.c | 6 ++++-- drivers/net/tun_vnet.c | 14 +++++++++----- drivers/net/tun_vnet.h | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-)