Re: [PATCH net-next 2/9] net/mlx5e: check for failure when pulling the Ethernet header after XDP

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In mlx5e_skb_from_cqe_mpwrq_nonlinear()
  (drivers/net/ethernet/mellanox/mlx5/core/en_rx.c), the new drop path…

--

net/mlx5e: check for failure when pulling the Ethernet header after XDP

This patch replaces the unchecked __pskb_pull_tail() call in
mlx5e_skb_from_cqe_mpwrq_nonlinear() with pskb_may_pull(). If the pull
fails, the packet is dropped. The goal is to stop eth_type_trans() from
hitting BUG() in __skb_pull() when the linear part holds less than
ETH_HLEN.

> When an XDP program leaves less than ETH_HLEN in the linear part of a
> multi-buffer packet, mlx5e_skb_from_cqe_mpwrq_nonlinear() pulls the rest
> of the Ethernet header in from the frags with __pskb_pull_tail() and
> ignores the return value.  If that pull fails, eth_type_trans() later
> finds less than ETH_HLEN in the head and BUG()s in __skb_pull().

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index e3f915beebe1b..b1e1e30a77e22 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -2060,9 +2060,12 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
>  				pagep->frags++;
>  			while (++pagep < frag_page);
>  
> -			if (len < ETH_HLEN)
> -				__pskb_pull_tail(skb, min(ETH_HLEN - len,
> -							  skb->data_len));
> +			if (len < ETH_HLEN &&
> +			    !pskb_may_pull(skb, min(ETH_HLEN, skb->len))) {
> +				rq->stats->buff_alloc_err++;
> +				dev_kfree_skb_any(skb);
> +				return NULL;
> +			}

[Severity: Low]
Can this new drop path actually be reached? Given how the skb is built
on the XDP path, it doesn't look like the pull can fail.

For an skb with frags and no frag_list, these are the only failure
returns in __pskb_pull_tail():

net/core/skbuff.c:__pskb_pull_tail() {
    ...
	if (!skb_frags_readable(skb))
		return NULL;

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
			return NULL;
	}
    ...
}

The frags are always readable here because XDP and memory providers
can't be used together:

- dev_xdp_attach() returns -EBUSY when dev_get_min_mp_channel_count()
  is non-zero.
- The memory provider binding code in netdev_rx_queue.c refuses when
  dev_xdp_prog_count() is non-zero.

The skb isn't cloned either, since mlx5e_build_linear_skb() has just
built it with napi_build_skb().

There is also always enough tailroom, so eat stays negative. The XDP
setup earlier in this function is:

	linear_hr = XDP_PACKET_HEADROOM;
	linear_data_len = 0;
	linear_frame_sz = MLX5_SKB_FRAG_SZ(linear_hr + MLX5E_RX_MAX_HEAD);

That leaves about 256 bytes past the linear tail, and the pull is never
more than ETH_HLEN bytes.

On the commit message's claim that this happens "When an XDP program
leaves less than ETH_HLEN in the linear part of a multi-buffer packet",
is that accurate?

The driver always gives XDP an empty linear area. So the pull runs for
every multi-buffer XDP_PASS packet whose program doesn't adjust the
head.

An XDP program also can't leave 1 to 13 bytes in the linear part,
because bpf_xdp_adjust_head() rejects it:

	if (unlikely(data < data_start ||
		     data > xdp->data_end - ETH_HLEN))
		return -EINVAL;

When frags are present, bpf_xdp_adjust_tail() only changes the frags,
so the linear data_end stays where it is. That means len is either 0 or
at least ETH_HLEN at this point.

If this change is meant as hardening, for example against later changes
to linear_frame_sz, could the commit message say so?

If there is a real way to make the pull fail, could the message describe
it and add a Fixes: tag for the commit that added the unchecked
__pskb_pull_tail()?

>  		}
>  	} else {
>  		if (xdp_buff_has_frags(&mxbuf->xdp)) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com