Re: [PATCH v2 1/3] tun: Unify vnet implementation
From: Akihiko Odaki
Date: Fri Jan 10 2025 - 03:29:05 EST
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.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
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)