Re: [PATCH RFC net-next v2 3/4] xdp: split generic XDP skb handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] xdp: netif_skb_check_for_xdp() drops shared skbs in
  pskb_expand_head()
- [Medium] xdp: netif_receive_generic_xdp() mutates shared skb before
  CoW
--

commit 154810a21d3faf42403d1f38f7f61c4add271315
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

xdp: split generic XDP skb handling

This commit introduces __do_xdp_generic() and struct xdp_generic_ctx so
callers can supply their own xdp_buff, page_pool, and xdp_rxq_info, and
adds skb_shared(skb) to the generic XDP copy-on-write check via
skb_needs_xdp_cow().

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 12595f6b9eba7..81848e638173b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[ ... ]

> @@ -5774,15 +5779,31 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	return act;
>  }
>  
> +u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
> +			     const struct bpf_prog *xdp_prog)
> +{
> +	return __bpf_prog_run_generic_xdp(skb, xdp, xdp_prog, NULL);
> +}
> +
>  static int
> -netif_skb_check_for_xdp(struct sk_buff **pskb, const struct bpf_prog *prog)
> +netif_skb_check_for_xdp(struct sk_buff **pskb, const struct bpf_prog *prog,
> +			struct page_pool *page_pool)
>  {
>  	struct sk_buff *skb = *pskb;
>  	int err, hroom, troom;
>  
> -	local_lock_nested_bh(&system_page_pool.bh_lock);
> -	err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool), pskb, prog);
> -	local_unlock_nested_bh(&system_page_pool.bh_lock);
> +	if (page_pool) {
> +		/* veth, the caller with its own page_pool, limits the MTU so
> +		 * that the copy is linear even for programs without frags
> +		 * support, and always copied into its page_pool.
> +		 */
> +		err = skb_pp_cow_data(page_pool, pskb, XDP_PACKET_HEADROOM);
> +	} else {
> +		local_lock_nested_bh(&system_page_pool.bh_lock);
> +		err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool),
> +					   pskb, prog);
> +		local_unlock_nested_bh(&system_page_pool.bh_lock);
> +	}
>  	if (!err)
>  		return 0;
>  
> @@ -5800,9 +5821,22 @@ netif_skb_check_for_xdp(struct sk_buff **pskb, const struct bpf_prog *prog)
>  	return skb_linearize(skb);
>  }
>  
> +/* Run the program on a private, page_pool-backed copy when it could otherwise
> + * change data someone else sees (shared or cloned skb), when frags have to be
> + * page_pool-backed so that XDP helpers can release them, or when the XDP
> + * headroom contract is not met. The head does not have to be a page frag:
> + * skb-backed XDP never turns the head into an xdp_frame.
> + */
> +static bool skb_needs_xdp_cow(const struct sk_buff *skb)
> +{
> +	return skb_shared(skb) || skb_cloned(skb) || skb_is_nonlinear(skb) ||
> +	       skb_headroom(skb) < XDP_PACKET_HEADROOM;
> +}

[Severity: Medium]
Does adding skb_shared(skb) here cause do_xdp_generic() to warn and drop
shared skbs when running a non-frags XDP program?

When page_pool is NULL (as in do_xdp_generic()) and
!prog->aux->xdp_has_frags, skb_cow_data_for_xdp() returns -EINVAL without
copying the skb.  netif_skb_check_for_xdp() then falls back to
pskb_expand_head(skb, ...), which rejects shared skbs with
DEBUG_NET_WARN_ON_ONCE(1) and -EINVAL:

net/core/skbuff.c:pskb_expand_head() {
	...
	if (unlikely(skb_shared(skb))) {
		DEBUG_NET_WARN_ON_ONCE(1);
		return -EINVAL;
	}
	...
}

Any shared skb (for example from pktgen with F_SHARED or after a packet
tap takes a reference before an rx_handler returns RX_HANDLER_ANOTHER) is
then dropped at do_drop instead of being unshared.

>  static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
>  				     struct xdp_buff *xdp,
> -				     const struct bpf_prog *xdp_prog)
> +				     const struct bpf_prog *xdp_prog,
> +				     struct xdp_generic_ctx *ctx)
>  {
>  	struct sk_buff *skb = *pskb;
>  	u32 mac_len, act = XDP_DROP;
> @@ -5820,15 +5854,17 @@ static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
>  	mac_len = skb->data - skb_mac_header(skb);
>  	__skb_push(skb, mac_len);
>  
> -	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
> -	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> -		if (netif_skb_check_for_xdp(pskb, xdp_prog))
> +	if (skb_needs_xdp_cow(skb)) {
> +		if (netif_skb_check_for_xdp(pskb, xdp_prog, ctx->page_pool))
>  			goto do_drop;
>  	}
>  
>  	__skb_pull(*pskb, mac_len);

[Severity: Medium]
Doesn't __skb_push(skb, mac_len) before skb_needs_xdp_cow() corrupt
skb->data and skb->len for other holders of a shared skb?

Unlike a cloned skb, a shared skb shares the struct sk_buff itself.  When
skb_pp_cow_data() replaces *pskb with a newly allocated nskb and calls
consume_skb(skb), the subsequent __skb_pull(*pskb, mac_len) only pulls
mac_len on nskb (and is skipped altogether on the goto do_drop path),
leaving the original shared skb with skb->data pushed back by mac_len and
skb->len increased by mac_len for any remaining reference holders.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110101941.1059662-1-sst@poczta.fm?part=3