On 2025/01/09 23:06, Willem de Bruijn wrote:
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.hTURBOCHANNEL SUBSYSTEMM: "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_VNETNo need for this new Kconfig
I will merge tun_vnet.c into TAP.
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?
No, I'm not sure what this check is for, but it is irrlevant with vnet header and shouldn't be modified with this patch. I'll restore the check with the next version.
+ 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).
I will do so.
This is subtle code. Please report what tests you ran to ensure that it does not introduce behavioral changes / regressions.
I tried: - curl on QEMU with macvtap (vhost=on) - curl on QEMU with macvtap (vhost=off)
