Re: [PATCH net-next 3/3] net/mlx5e: use skb_share_check() in the loopback selftest hook

[netdev-bot+sashiko@kernel.org]

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_test_loopback_validate()
  (drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c), the comment…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  mlx5e_test_loopback_validate()…

--

net/mlx5e: use skb_share_check() in the loopback selftest hook

This commit adds a skb_share_check() call in
mlx5e_test_loopback_validate() ahead of skb_linearize(). With it, the
loopback selftest hook no longer linearizes an skb that another
ETH_P_IP handler bound to the device also holds.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c b/drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c
> index 1dcdb86690bb7..8eb9a9469549d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c
> @@ -166,6 +166,10 @@ mlx5e_test_loopback_validate(struct sk_buff *skb,
>  	struct udphdr *udph;
>  	struct iphdr *iph;
>  
> +	skb = skb_share_check(skb, GFP_ATOMIC);
> +	if (!skb)
> +		goto out;
> +
>  	if (skb_linearize(skb))
>  		goto out;
>  

[Severity: Low]
This isn't a bug, but the comment just below this hunk in
mlx5e_test_loopback_validate() now contradicts the new code:

	if (skb_linearize(skb))
		goto out;

	/* We are only going to peek, no need to clone the SKB */
	if (MLX5E_TEST_PKT_SIZE - ETH_HLEN > skb_headlen(skb))
		goto out;

skb_share_check() calls skb_clone() whenever the skb is shared. After
that, skb_linearize() can modify or reallocate the skb head.

Should this comment be dropped or reworded? A later reader could take
"no need to clone" at face value and remove the skb_share_check().
stmmac_test_loopback_validate() uses the same skb_share_check() +
skb_linearize() sequence and has no such comment.

The commit message also says:

  skb_linearize() on a non-linear frame then hits BUG_ON(skb_shared()).

Is this accurate for mlx5 in general? __pskb_pull_tail() only calls
pskb_expand_head(), which holds the BUG_ON(skb_shared()), under this
condition:

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))

Both mlx5 non-linear RX builders seem to leave enough tailroom for this
small test frame's frags. Without XDP,
mlx5e_skb_from_cqe_mpwrq_nonlinear() uses
napi_alloc_skb(ALIGN(MLX5E_RX_MAX_HEAD)). With XDP it uses
MLX5_SKB_FRAG_SZ(XDP_PACKET_HEADROOM + MLX5E_RX_MAX_HEAD).

So on mlx5 the BUG_ON seems to need an skb that is both shared and
cloned. Could the commit message state the triggering condition more
precisely? The fix itself looks correct, since users == 1 after
skb_share_check().

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch.
Further down in mlx5e_test_loopback_validate(), the UDP header is
located using iph->ihl, which comes from the packet and is never
checked:

	/* Don't assume skb_transport_header() was set */
	udph = (struct udphdr *)((u8 *)iph + 4 * iph->ihl);
	if (udph->dest != htons(9))
		goto out;

	mlxh = (struct mlx5ehdr *)((char *)udph + sizeof(*udph));
	if (mlxh->magic != cpu_to_be64(MLX5E_TEST_MAGIC))

The only length check is this one:

	if (MLX5E_TEST_PKT_SIZE - ETH_HLEN > skb_headlen(skb))
		goto out;

That guarantees 44 bytes: 20 for IP, 8 for UDP, and 16 for struct
mlx5ehdr, which includes the padding before magic.

The hook is registered with dev_add_pack() for every ETH_P_IP frame on
priv->netdev. While the loopback test is running, could an external UDP
frame sent to the port MAC with ihl up to 15 make this code read
udph->dest and mlxh->magic up to 40 bytes past skb_headlen()?

Those bytes stay inside the skb head allocation, in the tailroom or
skb_shared_info, and they are only compared against constants. The
result could be a spurious selftest outcome or a KMSAN uninit-value
report. Would it make sense to check 4 * iph->ihl against skb_headlen()
before using it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-nb-net-selftest-shared-net-v1-0-458369bfa1ba%40tipi-net.de