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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> 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.
