Re: [PATCH net v2] net: tun: bound receive headroom

On Thursday, August 6th, 2026 at 7:22 AM, Jakub Kicinski [off-list ref] wrote:

This is an AI-generated review of your patch. The human sending this
email says "This is what the bots think :)"
---
net: tun: bound receive headroom

This patch clamps the headroom that tun_set_headroom() stores in tun->align
so that SKB_MAX_HEAD(align) in tun_get_user() cannot underflow, and it adds
pskb_may_pull() calls before reading the raw-TUN first byte and the TAP
Ethernet header.

quoted

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fed9dfdfcc3bc..efd2e7d75c9ac 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c

@@ -1107,11 +1107,16 @@ static netdev_features_t tun_net_fix_features(struct net_device *dev,
 static void tun_set_headroom(struct net_device *dev, int new_hr)
 {
 	struct tun_struct *tun = netdev_priv(dev);
+	size_t max_headroom;

-	if (new_hr < NET_SKB_PAD)
-		new_hr = NET_SKB_PAD;
+	max_headroom = min_t(size_t, SKB_MAX_HEAD(0), U16_MAX - 1);

-	tun->align = new_hr;
+	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
+		max_headroom -= ETH_HLEN + NET_IP_ALIGN;
+	else
+		max_headroom -= 1;
+
+	tun->align = clamp_t(int, new_hr, NET_SKB_PAD, max_headroom);
 }

[Severity: Low]
Is the upper bound intentionally this tight?  At the clamp maximum,
tun_get_user() computes:

	good_linear = SKB_MAX_HEAD(align);

which evaluates to 1 for raw TUN and ETH_HLEN for TAP.  The non-zerocopy
path then does:

	copylen = len;
	linear = min(hdr_len, good_linear);

so the resulting skb has skb_headlen() of 1 (or 14) bytes.

Does that make every write() carrying a virtio_net_hdr with
VIRTIO_NET_HDR_F_NEEDS_CSUM or a GSO type fail?

I don't really follow this; not for the linear-area reason described. The
virtio-header conversion calls pskb_may_pull() for the required checksum
or GSO headers before skb_partial_csum_set(). The requested pull length
includes the checksum field, so after a successful pull csum_end is within
skb_headlen(). The one-byte and ETH_HLEN values are only the minimum
prefix initially kept linear.

In configs where the 16-bit (vs the one-page) offset limit controls the
cap, an extreme max-headroom request can still leave too little room for
a non-zero checksum-start offset to fit. But that is rejected safely and
is a whole other topic.

The chain
tun_vnet_hdr_tnl_to_skb() -> virtio_net_hdr_to_skb() ends in
skb_partial_csum_set(), which requires the csum offsets to live inside the
linear area:

net/core/skbuff.c:skb_partial_csum_set() {
	...
	if (unlikely(csum_start >= U16_MAX || csum_end > skb_headlen(skb))) {
		net_warn_ratelimited("bad partial csum: ...");
		return false;
	}
	...
}

With a 1-byte or 14-byte headlen that check always fails, so tun_get_user()
takes the err = -EINVAL path, bumps rx_frame_errors and drops the frame for
all checksum-offloaded and GSO traffic while the headroom request is in
effect.

Note that for IFF_TUN, tun_can_build_skb() returns false, so the raw-TUN
path always goes through tun_alloc_skb() and always sees good_linear == 1 at
the bound.  tun_alloc_skb() only overrides linear when prepad + len <
PAGE_SIZE, which is not the case for non-trivial packets at this prepad.

Would reserving a usable linear area instead (for example GOODCOPY_LEN, or
the maximum hdr_len the device can accept) give the same memory-safety
guarantee without turning offloaded traffic into drops?  For a Cc: stable
patch, could the commit message also mention this consequence?  Right now it
only says:

  "Leave one linear byte for raw TUN and a complete Ethernet header for TAP,
   including NET_IP_ALIGN."

quoted

@@ -1822,7 +1827,13 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case IFF_TUN:
 		if (tun->flags & IFF_NO_PI) {
-			u8 ip_version = skb->len ? (skb->data[0] >> 4) : 0;
+			u8 ip_version;
+
+			if (skb->len && !pskb_may_pull(skb, 1)) {
+				err = -ENOMEM;
+				goto drop;
+			}

[Severity: Low]
Should this path set drop_reason too?

I'll just do what Willem suggested in v3 instead:

	if (!pskb_may_pull(skb, 1)) {
		err = -EINVAL;
		goto drop;
	}

	ip_version = skb->data[0] >> 4;

So there'd be no skb->len check, so pull failure can also mean
that the packet contains no first byte, at which point SKB_DROP_REASON_NOMEM
wouldn't make sense either. So I think it's best not to set it?

It jumps to drop without touching
drop_reason, so the free_skb label in tun_get_user() reports the initial
SKB_DROP_REASON_NOT_SPECIFIED:

drivers/net/tun.c:tun_get_user() {
	...
free_skb:
	if (!IS_ERR_OR_NULL(skb))
		kfree_skb_reason(skb, drop_reason);
	...
}

Since the call is guarded by skb->len, pskb_may_pull(skb, 1) can only fail
via the GFP_ATOMIC allocation inside __pskb_pull_tail()/pskb_expand_head(),
so SKB_DROP_REASON_NOMEM looks like the accurate value here.

The IFF_TAP arm changed by the same hunk does set an explicit reason for its
pull failure, so the two adjacent sites in the same switch end up reporting
different things to the kfree_skb_reason() tracepoint.

quoted

+			ip_version = skb->len ? (skb->data[0] >> 4) : 0;

 			switch (ip_version) {
 			case 4:

[ ... ]

quoted

@@ -1842,7 +1853,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		skb->dev = tun->dev;
 		break;
 	case IFF_TAP:
-		if (frags && !pskb_may_pull(skb, ETH_HLEN)) {
+		if (!pskb_may_pull(skb, ETH_HLEN)) {
 			err = -ENOMEM;
 			drop_reason = SKB_DROP_REASON_HDR_TRUNC;
 			goto drop;

--
pw-bot: cr