Re: [PATCH v2 1/3] tun: Unify vnet implementation

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Akihiko Odaki wrote:
> Both tun and tap exposes the same set of virtio-net-related features.
> Unify their implementations to ease future changes.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  MAINTAINERS            |   1 +
>  drivers/net/Kconfig    |   5 ++
>  drivers/net/Makefile   |   1 +
>  drivers/net/tap.c      | 172 ++++++----------------------------------
>  drivers/net/tun.c      | 208 ++++++++-----------------------------------------
>  drivers/net/tun_vnet.c | 186 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/tun_vnet.h |  24 ++++++
>  7 files changed, 273 insertions(+), 324 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 910305c11e8a..1be8a452d11f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23903,6 +23903,7 @@ F:	Documentation/networking/tuntap.rst
>  F:	arch/um/os-Linux/drivers/
>  F:	drivers/net/tap.c
>  F:	drivers/net/tun.c
> +F:	drivers/net/tun_vnet.h
>  
>  TURBOCHANNEL SUBSYSTEM
>  M:	"Maciej W. Rozycki" <macro@orcam.me.uk>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 1fd5acdc73c6..255c8f9f1d7c 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -395,6 +395,7 @@ config TUN
>  	tristate "Universal TUN/TAP device driver support"
>  	depends on INET
>  	select CRC32
> +	select TUN_VNET

No need for this new Kconfig

>  static struct proto tap_proto = {
>  	.name = "tap",
> @@ -641,10 +576,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  	struct sk_buff *skb;
>  	struct tap_dev *tap;
>  	unsigned long total_len = iov_iter_count(from);
> -	unsigned long len = total_len;
> +	unsigned long len;
>  	int err;
>  	struct virtio_net_hdr vnet_hdr = { 0 };
> -	int vnet_hdr_len = 0;
> +	int hdr_len;
>  	int copylen = 0;
>  	int depth;
>  	bool zerocopy = false;
> @@ -652,38 +587,20 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  	enum skb_drop_reason drop_reason;
>  
>  	if (q->flags & IFF_VNET_HDR) {
> -		vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
> -
> -		err = -EINVAL;
> -		if (len < vnet_hdr_len)
> -			goto err;
> -		len -= vnet_hdr_len;
> -
> -		err = -EFAULT;
> -		if (!copy_from_iter_full(&vnet_hdr, sizeof(vnet_hdr), from))
> -			goto err;
> -		iov_iter_advance(from, vnet_hdr_len - sizeof(vnet_hdr));
> -		if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> -		     tap16_to_cpu(q, vnet_hdr.csum_start) +
> -		     tap16_to_cpu(q, vnet_hdr.csum_offset) + 2 >
> -			     tap16_to_cpu(q, vnet_hdr.hdr_len))
> -			vnet_hdr.hdr_len = cpu_to_tap16(q,
> -				 tap16_to_cpu(q, vnet_hdr.csum_start) +
> -				 tap16_to_cpu(q, vnet_hdr.csum_offset) + 2);
> -		err = -EINVAL;
> -		if (tap16_to_cpu(q, vnet_hdr.hdr_len) > len)
> +		hdr_len = tun_vnet_hdr_get(READ_ONCE(q->vnet_hdr_sz), q->flags, from, &vnet_hdr);
> +		if (hdr_len < 0) {
> +			err = hdr_len;
>  			goto err;
> +		}
> +	} else {
> +		hdr_len = 0;
>  	}
>  
> -	err = -EINVAL;
> -	if (unlikely(len < ETH_HLEN))
> -		goto err;
> -

Is this check removal intentional?

> +	len = iov_iter_count(from);
>  	if (msg_control && sock_flag(&q->sk, SOCK_ZEROCOPY)) {
>  		struct iov_iter i;
>  
> -		copylen = vnet_hdr.hdr_len ?
> -			tap16_to_cpu(q, vnet_hdr.hdr_len) : GOODCOPY_LEN;
> +		copylen = hdr_len ? hdr_len : GOODCOPY_LEN;
>  		if (copylen > good_linear)
>  			copylen = good_linear;
>  		else if (copylen < ETH_HLEN)
> @@ -697,7 +614,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  
>  	if (!zerocopy) {
>  		copylen = len;
> -		linear = tap16_to_cpu(q, vnet_hdr.hdr_len);
> +		linear = hdr_len;
>  		if (linear > good_linear)
>  			linear = good_linear;
>  		else if (linear < ETH_HLEN)
> @@ -732,9 +649,8 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  	}
>  	skb->dev = tap->dev;
>  
> -	if (vnet_hdr_len) {
> -		err = virtio_net_hdr_to_skb(skb, &vnet_hdr,
> -					    tap_is_little_endian(q));
> +	if (q->flags & IFF_VNET_HDR) {
> +		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
>  		if (err) {
>  			rcu_read_unlock();
>  			drop_reason = SKB_DROP_REASON_DEV_HDR;
> @@ -797,23 +713,17 @@ static ssize_t tap_put_user(struct tap_queue *q,
>  	int total;
>  
>  	if (q->flags & IFF_VNET_HDR) {
> -		int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0;
>  		struct virtio_net_hdr vnet_hdr;
>  
>  		vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
> -		if (iov_iter_count(iter) < vnet_hdr_len)
> -			return -EINVAL;
> -
> -		if (virtio_net_hdr_from_skb(skb, &vnet_hdr,
> -					    tap_is_little_endian(q), true,
> -					    vlan_hlen))
> -			BUG();
>  
> -		if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) !=
> -		    sizeof(vnet_hdr))
> -			return -EFAULT;
> +		ret = tun_vnet_hdr_from_skb(q->flags, NULL, skb, &vnet_hdr);
> +		if (ret < 0)
> +			goto done;
>  
> -		iov_iter_advance(iter, vnet_hdr_len - sizeof(vnet_hdr));
> +		ret = tun_vnet_hdr_put(vnet_hdr_len, iter, &vnet_hdr);
> +		if (ret < 0)
> +			goto done;

Please split this patch in to a series of smaller patches.

If feasible:

1. one that move the head of tun.c into tun_vnet.[hc].
2. then one that uses that also in tap.c.
3. then a separate patch for the ioctl changes.
4. then introduce tun_vnet_hdr_from_skb, tun_vnet_hdr_put
and friends in (a) follow-up patch(es).

This is subtle code. Please report what tests you ran to ensure
that it does not introduce behavioral changes / regressions.