Re: [PATCH v2 1/4] net: tun: fix tun_xdp_one() for IFF_TUN mode

[Jason Wang <jasowang@redhat.com>]

在 2021/6/24 上午6:52, David Woodhouse 写道:
> On Wed, 2021-06-23 at 18:31 +0100, David Woodhouse wrote:
>> Joy... that's wrong because when tun does both the PI and the vnet
>> headers, the PI header comes *first*. When tun does only PI and vhost
>> does the vnet headers, they come in the other order.
>>
>> Will fix (and adjust the test cases to cope).
>
> I got this far, pushed to
> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/vhost-net
>
> All the test cases are now passing. I don't guarantee I haven't
> actually broken qemu and IFF_TAP mode though, mind you :)


No problem, but it would be easier for me if you can post another 
version of the series.


>
> I'll need to refactor the intermediate commits a little so I won't
> repost the series quite yet, but figured I should at least show what I
> have for comments, as my day ends and yours begins.
>
>
> As discussed, I expanded tun_get_socket()/tap_get_socket() to return
> the actual header length instead of letting vhost make wild guesses.


This probably won't work since we had TUNSETVNETHDRSZ.

I agree the vhost codes is tricky since it assumes only two kinds of the 
hdr length.

But it was basically how it works for the past 10 years. It depends on 
the userspace (Qemu) to coordinate it with the TUN/TAP through 
TUNSETVNETHDRSZ during the feature negotiation.


> Note that in doing so, I have made tun_get_socket() return -ENOTCONN if
> the tun fd *isn't* actually attached (TUNSETIFF) to a real device yet.


Any reason for doing this? Note that the socket is loosely coupled with 
the networking device.


>
> I moved the sanity check back to tun/tap instead of doing it in
> vhost_net_build_xdp(), because the latter has no clue about the tun PI
> header and doesn't know *where* the virtio header is.


Right, the deserves a separate patch.


>
>

[...]


>   	mutex_unlock(&n->dev.mutex);
> diff --git a/include/linux/if_tap.h b/include/linux/if_tap.h
> index 915a187cfabd..b460ba98f34e 100644
> --- a/include/linux/if_tap.h
> +++ b/include/linux/if_tap.h
> @@ -3,14 +3,14 @@
>   #define _LINUX_IF_TAP_H_
>   
>   #if IS_ENABLED(CONFIG_TAP)
> -struct socket *tap_get_socket(struct file *);
> +struct socket *tap_get_socket(struct file *, size_t *);
>   struct ptr_ring *tap_get_ptr_ring(struct file *file);
>   #else
>   #include <linux/err.h>
>   #include <linux/errno.h>
>   struct file;
>   struct socket;
> -static inline struct socket *tap_get_socket(struct file *f)
> +static inline struct socket *tap_get_socket(struct file *f, size_t *)
>   {
>   	return ERR_PTR(-EINVAL);
>   }
> diff --git a/include/linux/if_tun.h b/include/linux/if_tun.h
> index 2a7660843444..8d78b6bbc228 100644
> --- a/include/linux/if_tun.h
> +++ b/include/linux/if_tun.h
> @@ -21,11 +21,10 @@ struct tun_msg_ctl {
>   
>   struct tun_xdp_hdr {
>   	int buflen;
> -	struct virtio_net_hdr gso;


Any reason for doing this? I meant it can work but we need limit the 
changes that is unrelated to the fixes.

Thanks