Date:   Fri, 25 Jun 2021 13:00:40 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     David Woodhouse <dwmw2@...radead.org>, netdev@...r.kernel.org
Cc:     Eugenio Pérez <eperezma@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH v3 1/5] net: add header len parameter to tun_get_socket(),
 tap_get_socket()


在 2021/6/24 下午8:30, David Woodhouse 写道:
> From: David Woodhouse <dwmw@...zon.co.uk>
>
> The vhost-net driver was making wild assumptions about the header length
> of the underlying tun/tap socket.


It's by design to depend on the userspace to co-ordinate the vnet header 
setting with the underlying sockets.


>   Then it was discarding packets if
> the number of bytes it got from sock_recvmsg() didn't precisely match
> its guess.


Anything that is broken by this? The failure is a hint for the userspace 
that something is wrong during the coordination.


>
> Fix it to get the correct information along with the socket itself.


I'm not sure what is fixed by this. It looks to me it tires to let 
packet go even if the userspace set the wrong attributes to tap or 
vhost. This is even sub-optimal than failing explicitly fail the RX.


> As a side-effect, this means that tun_get_socket() won't work if the
> tun file isn't actually connected to a device, since there's no 'tun'
> yet in that case to get the information from.


This may break the existing application. Vhost-net is tied to the socket 
instead of the device that the socket is loosely coupled.


>
> On the receive side, where the tun device generates the virtio_net_hdr
> but VIRITO_NET_F_MSG_RXBUF was negotiated and vhost-net needs to fill
> in the 'num_buffers' field on top of the existing virtio_net_hdr, fix
> that to use 'sock_hlen - 2' as the location, which means that it goes
> in the right place regardless of whether the tun device is using an
> additional tun_pi header or not. In this case, the user should have
> configured the tun device with a vnet hdr size of 12, to make room.
>
> Fixes: 8dd014adfea6f ("vhost-net: mergeable buffers support")
> Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
> ---
>   drivers/net/tap.c      |  5 ++++-
>   drivers/net/tun.c      | 16 +++++++++++++++-
>   drivers/vhost/net.c    | 31 +++++++++++++++----------------
>   include/linux/if_tap.h |  4 ++--
>   include/linux/if_tun.h |  4 ++--
>   5 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 8e3a28ba6b28..2170a0d3d34c 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -1246,7 +1246,7 @@ static const struct proto_ops tap_socket_ops = {
>    * attached to a device.  The returned object works like a packet socket, it
>    * can be used for sock_sendmsg/sock_recvmsg.  The caller is responsible for
>    * holding a reference to the file for as long as the socket is in use. */
> -struct socket *tap_get_socket(struct file *file)
> +struct socket *tap_get_socket(struct file *file, size_t *hlen)
>   {
>   	struct tap_queue *q;
>   	if (file->f_op != &tap_fops)
> @@ -1254,6 +1254,9 @@ struct socket *tap_get_socket(struct file *file)
>   	q = file->private_data;
>   	if (!q)
>   		return ERR_PTR(-EBADFD);
> +	if (hlen)
> +		*hlen = (q->flags & IFF_VNET_HDR) ? q->vnet_hdr_sz : 0;
> +
>   	return &q->sock;
>   }
>   EXPORT_SYMBOL_GPL(tap_get_socket);
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 4cf38be26dc9..67b406fa0881 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3649,7 +3649,7 @@ static void tun_cleanup(void)
>    * attached to a device.  The returned object works like a packet socket, it
>    * can be used for sock_sendmsg/sock_recvmsg.  The caller is responsible for
>    * holding a reference to the file for as long as the socket is in use. */
> -struct socket *tun_get_socket(struct file *file)
> +struct socket *tun_get_socket(struct file *file, size_t *hlen)
>   {
>   	struct tun_file *tfile;
>   	if (file->f_op != &tun_fops)
> @@ -3657,6 +3657,20 @@ struct socket *tun_get_socket(struct file *file)
>   	tfile = file->private_data;
>   	if (!tfile)
>   		return ERR_PTR(-EBADFD);
> +
> +	if (hlen) {
> +		struct tun_struct *tun = tun_get(tfile);
> +		size_t len = 0;
> +
> +		if (!tun)
> +			return ERR_PTR(-ENOTCONN);
> +		if (tun->flags & IFF_VNET_HDR)
> +			len += READ_ONCE(tun->vnet_hdr_sz);
> +		if (!(tun->flags & IFF_NO_PI))
> +			len += sizeof(struct tun_pi);
> +		tun_put(tun);
> +		*hlen = len;
> +	}
>   	return &tfile->socket;
>   }
>   EXPORT_SYMBOL_GPL(tun_get_socket);
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index df82b124170e..b92a7144ed90 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1143,7 +1143,8 @@ static void handle_rx(struct vhost_net *net)
>   
>   	vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
>   		vq->log : NULL;
> -	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);
> +	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF) &&
> +		(vhost_hlen || sock_hlen >= sizeof(num_buffers));


So this change the behavior. When mergeable buffer is enabled, userspace 
expects the vhost to merge buffers. If the feature is disabled silently, 
it violates virtio spec.

If anything wrong in the setup, userspace just breaks itself.

E.g if sock_hlen is less that struct virtio_net_hdr_mrg_buf. The packet 
header might be overwrote by the vnet header.


>   
>   	do {
>   		sock_len = vhost_net_rx_peek_head_len(net, sock->sk,
> @@ -1213,9 +1214,10 @@ static void handle_rx(struct vhost_net *net)
>   			}
>   		} else {
>   			/* Header came from socket; we'll need to patch
> -			 * ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF
> +			 * ->num_buffers over the last two bytes if
> +			 * VIRTIO_NET_F_MRG_RXBUF is enabled.
>   			 */
> -			iov_iter_advance(&fixup, sizeof(hdr));
> +			iov_iter_advance(&fixup, sock_hlen - 2);


I'm not sure what did the above code want to fix. It doesn't change 
anything if vnet header is set correctly in TUN. It only prevents the 
the packet header from being rewrote.

Thanks

Powered by blists - more mailing lists