| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aeb2bd68-f5b9-49b7-81ff-18472bd8b7b0@daynix.com>
Date: Fri, 10 Jan 2025 17:28:38 +0900
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Jonathan Corbet <corbet@....net>, Jason Wang <jasowang@...hat.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo
<xuanzhuo@...ux.alibaba.com>, Shuah Khan <shuah@...nel.org>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, kvm@...r.kernel.org,
virtualization@...ts.linux-foundation.org, linux-kselftest@...r.kernel.org,
Yuri Benditovich <yuri.benditovich@...nix.com>,
Andrew Melnychenko <andrew@...nix.com>,
Stephen Hemminger <stephen@...workplumber.org>, gur.stavi@...wei.com,
devel@...nix.com
Subject: Re: [PATCH v2 1/3] tun: Unify vnet implementation
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@...nix.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@...am.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
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)
Powered by blists - more mailing lists