Re: [PATCH] net: skbuff: don't BUG() when skb_copy_bits() fails in __pskb_pull_tail

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

shao.mingyin@ wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> __pskb_pull_tail() BUG()s if skb_copy_bits() returns an error.
> skb_copy_bits() fails when the lengths do not add up, or when a
> frag_list child carries unreadable frags (devmem net_iov, commit
> 65249feb6b3d ("net: add support for skbs with unreadable frags")).  The
> top-level check above does not cover frag_list children, and
> skb_gro_receive() links them without propagating skb->unreadable, so a
> mixed skb trips the BUG().
> 
> Nothing has been committed at that point, so return NULL the way the
> rest of the function already handles failures, with a
> DEBUG_NET_WARN_ON_ONCE() for debug kernels.  skb_condense(), the one
> net/core caller that ignores the return, now checks it and keeps the
> original truesize when the pull fails, as the frags are still there.
> 
> The other direct callers that ignore the return do not build a
> frag_list child: xen-netfront drops the skb when the failed pull leaves
> nr_frags at MAX_SKB_FRAGS, and niu, mlx5 and ftmac100 pull at most what
> their own skb holds.  A length mismatch there is a driver bug, which
> the debug warning reports.
> 
> This is the separate change that commit 279877d86c82 ("net: skbuff:
> don't BUG() when skb_copy_bits() fails in copy helpers") left for the
> callers; the audit is above.  No Fixes: tag and no stable marking: the
> mixed skb is an invariant violation, not a regression, and this is a
> net-next hardening in the BUG_ON() removal series.

In the series, Josef reported

  Two BUG_ON()s are left on purpose.  __pskb_pull_tail() and
  skb_pull_rcsum() have callers that can't otherwise fail, so they don't
  check the return.  Some of them would carry on and BUG() somewhere
  else, or push back a pull that never happened.  Those need their
  callers fixed first and will come as separate series.

> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
>  net/core/skbuff.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Tested on 7.3.0-next-20261030 (QEMU/TCG guest).  QEMU has no devmem
> (net_iov) backend, so a TEST-ONLY hook (applied to both kernels, not
> part of this patch) fabricates an skb whose frag_list child carries an
> unreadable frag, then calls the real __pskb_pull_tail() on it at
> late_initcall time: skb_copy_bits() recurses into the child, sees the
> unreadable frag and fails.  The hook then calls skb_condense() on the
> same skb, so the failed pull must not shrink truesize.
> 
>   unpatched                             patched
>   kernel BUG at                         __pskb_pull_tail() returns NULL,
>   net/core/skbuff.c:2903                skb_condense() keeps truesize 960,
>                                         boot continues, no BUG
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f676411724..8a200252ef93 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2900,8 +2900,11 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta)
>  			return NULL;
>  	}
>  
> -	BUG_ON(skb_copy_bits(skb, skb_headlen(skb),
> -			     skb_tail_pointer(skb), delta));
> +	if (unlikely(skb_copy_bits(skb, skb_headlen(skb),
> +				   skb_tail_pointer(skb), delta))) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return NULL;
> +	}
>  
>  	/* Optimization: no fragments, no reasons to preestimate
>  	 * size of pulled pages. Superb.
> @@ -7218,7 +7221,8 @@ void skb_condense(struct sk_buff *skb)
>  			return;
>  
>  		/* Nice, we can free page frag(s) right now */
> -		__pskb_pull_tail(skb, skb->data_len);
> +		if (!__pskb_pull_tail(skb, skb->data_len))
> +			return;	/* frags are still there, keep truesize */
>  	}
>  	/* At this point, skb->truesize might be over estimated,
>  	 * because skb had a fragment, and fragments do not tell
> --