[PATCH net-next 0/3] tun: trim per-packet work on the write path

[PATCH net-next 0/3] tun: trim per-packet work on the write path

[Antonio Quartulli]

Profiling a link-bonding daemon on an embedded router (Cortex-A7), which
injects every packet with a write() to a tun device, showed tun doing
per-packet work its packets do not need:

1. a flow dissector run, only to set the transport header
2. a BH-disabled RCU section, only to check for an XDP program, which
   only TAP devices can have
3. the parsing of an all-zero virtio-net header without IFF_VNET_HDR

Per 1400-byte packet written and dropped at the raw table, instruction
counts (qemu -icount) drop by 6.5% for 1 (Cortex-A7), 2.3% for 2 and
1.6% for 3 (x86). The tun selftests pass.

Antonio Quartulli (3):
  tun: don't probe the transport header of packets written by userspace
  tun: don't disable BHs for the XDP program check of every packet
  tun: skip the virtio-net header parsing without IFF_VNET_HDR

 drivers/net/tun.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

-- 
2.56.0

[PATCH net-next 1/3] tun: don't probe the transport header of packets written by userspace

[Antonio Quartulli]

tun_get_user() runs the flow dissector on every written packet only to
set the transport header. Packets that need it early already get it:
skb_partial_csum_set() for NEEDS_CSUM, __virtio_net_hdr_to_skb() for
GSO. For the rest, the IPv4/IPv6 receive paths set it, as for packets
from NICs.

Drop the probe: ~200 fewer instructions per 1400-byte packet, -6.5%
when dropped at the raw table, -2.4% through conntrack and NAT
(emulated Cortex-A7).

Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 drivers/net/tun.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 8920c797acfd..9fc60a73fb96 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1960,7 +1960,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		net_zcopy_complete(NULL, msg_control, false);
 
 	skb_reset_network_header(skb);
-	skb_probe_transport_header(skb);
 	skb_record_rx_queue(skb, tfile->queue_index);
 
 	if (skb_xdp) {
-- 
2.56.0

[PATCH net-next 2/3] tun: don't disable BHs for the XDP program check of every packet

[Antonio Quartulli]

For every packet, tun_get_user() disables BHs and enters an RCU section
only to find out whether an XDP program is attached, which only TAP
devices can have.

Check with rcu_access_pointer() first, in tun_build_skb() too, and enter
the BH-disabled RCU section only with a program attached. A program
attached concurrently could already miss the packets in flight.

-2.3% instructions per 1400-byte packet dropped at the raw table, -3.0%
when written 64 per write() (emulated x86).

Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 drivers/net/tun.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9fc60a73fb96..395923651f31 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1702,13 +1702,12 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 	int pad = TUN_RX_PAD;
 	int metasize = 0;
 	int err = 0;
+	bool xdp;
 
-	rcu_read_lock();
-	xdp_prog = rcu_dereference(tun->xdp_prog);
-	if (xdp_prog)
+	xdp = rcu_access_pointer(tun->xdp_prog);
+	if (xdp)
 		pad += XDP_PACKET_HEADROOM;
 	buflen += SKB_DATA_ALIGN(len + pad);
-	rcu_read_unlock();
 
 	alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
 	if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, GFP_KERNEL)))
@@ -1725,7 +1724,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 	 * of xdp_prog above, this should be rare and for simplicity
 	 * we do XDP on skb in case the headroom is not enough.
 	 */
-	if (hdr->gso_type || !xdp_prog) {
+	if (hdr->gso_type || !xdp) {
 		*skb_xdp = 1;
 		return __tun_build_skb(tfile, alloc_frag, buf, buflen, len,
 				       pad, metasize);
@@ -1962,7 +1961,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	skb_reset_network_header(skb);
 	skb_record_rx_queue(skb, tfile->queue_index);
 
-	if (skb_xdp) {
+	/* Only TAP devices can have an XDP program: don't disable BHs and
+	 * enter an RCU section for every packet just to find out it's unset.
+	 */
+	if (skb_xdp && rcu_access_pointer(tun->xdp_prog)) {
 		struct bpf_prog *xdp_prog;
 		int ret;
 
-- 
2.56.0

[PATCH net-next 3/3] tun: skip the virtio-net header parsing without IFF_VNET_HDR

[Antonio Quartulli]

Without IFF_VNET_HDR, tun_get_user() still passes an all-zero header to
tun_vnet_hdr_tnl_to_skb(), which then only resets the MAC header. Do
just that.

-1.6% instructions per 1400-byte packet dropped at the raw table, -2.2%
when written 64 per write() (emulated x86).

Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 drivers/net/tun.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 395923651f31..38f9da1ab462 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1943,7 +1943,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;
 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	if (!(tun->flags & IFF_VNET_HDR)) {
+		/* all a zeroed header would do */
+		skb_reset_mac_header(skb);
+	} else if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
-- 
2.56.0

Re: [PATCH net-next 2/3] tun: don't disable BHs for the XDP program check of every packet

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] tun: ubuf_info reference leak in tun_get_user() [1]
--

commit 1a1e58a937e3b55d0632ff554e804970bf806fab
Author: Antonio Quartulli <antonio@mandelbit.com>

tun: don't disable BHs for the XDP program check of every packet

This commit checks tun->xdp_prog with rcu_access_pointer() in
tun_build_skb() and tun_get_user() so packets on devices without an XDP
program avoid entering an RCU read-side critical section or disabling BHs.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9fc60a73fb963..395923651f312 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1702,13 +1702,12 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  	int pad = TUN_RX_PAD;
>  	int metasize = 0;
>  	int err = 0;
> +	bool xdp;
>  
> -	rcu_read_lock();
> -	xdp_prog = rcu_dereference(tun->xdp_prog);
> -	if (xdp_prog)
> +	xdp = rcu_access_pointer(tun->xdp_prog);
> +	if (xdp)
>  		pad += XDP_PACKET_HEADROOM;
>  	buflen += SKB_DATA_ALIGN(len + pad);
> -	rcu_read_unlock();
>  
>  	alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
>  	if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, GFP_KERNEL)))
> @@ -1725,7 +1724,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  	 * of xdp_prog above, this should be rare and for simplicity
>  	 * we do XDP on skb in case the headroom is not enough.
>  	 */
> -	if (hdr->gso_type || !xdp_prog) {
> +	if (hdr->gso_type || !xdp) {
>  		*skb_xdp = 1;
>  		return __tun_build_skb(tfile, alloc_frag, buf, buflen, len,
>  				       pad, metasize);
> @@ -1962,7 +1961,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	skb_reset_network_header(skb);
>  	skb_record_rx_queue(skb, tfile->queue_index);
>  
> -	if (skb_xdp) {
> +	/* Only TAP devices can have an XDP program: don't disable BHs and
> +	 * enter an RCU section for every packet just to find out it's unset.
> +	 */
> +	if (skb_xdp && rcu_access_pointer(tun->xdp_prog)) {
>  		struct bpf_prog *xdp_prog;
>  		int ret;
>  

[Severity: High]
This problem wasn't introduced by this patch, but does tun_get_user() leak
the msg_control ubuf_info reference when tun_build_skb() consumes the
packet via XDP [1]?

When msg_control is non-NULL but iov_iter_npages() exceeds MAX_SKB_FRAGS,
zerocopy stays false and tun_can_build_skb() can succeed:

drivers/net/tun.c:tun_get_user() {
    ...
	if (msg_control) {
		...
		if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
			zerocopy = true;
	}

	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
		skb = tun_build_skb(tun, tfile, from, gso, len, &skb_xdp);
		err = PTR_ERR_OR_ZERO(skb);
		if (err)
			goto drop;
		if (!skb)
			return total_len;
	}
    ...
	if (zerocopy)
		skb_zcopy_init(skb, msg_control);
	else if (msg_control)
		net_zcopy_complete(NULL, msg_control, false);
}

If the XDP program in tun_build_skb() returns XDP_DROP, XDP_TX, or
XDP_REDIRECT, tun_build_skb() returns NULL and tun_get_user() returns
total_len without calling net_zcopy_complete().  That permanently leaks the
ubuf_info reference, which exhausts vhost_net virtqueue descriptors and
hangs vhost_net_ubuf_put_wait_and_free() on teardown.

[1] https://sashiko.dev/#/bug/linux-f9997fb6-303f-41d6-93e2-60c392b1856f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112150612.3866546-1-antonio@mandelbit.com?part=2