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

On Thursday, August 6th, 2026 at 7:22 AM, Jakub Kicinski <[email protected]> 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.
> 
> > 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."
> 
> > @@ -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.
> 
> > +			ip_version = skb->len ? (skb->data[0] >> 4) : 0;
> >
> >  			switch (ip_version) {
> >  			case 4:
> 
> [ ... ]
> 
> > @@ -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
>