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

[Nicolai Buchwitz]

On 8.10.2026 23:31, netdev-bot+sashiko@xxxxxxxxxx wrote:

> [...]

> [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.

Yes, it is stale. I'll drop it if a v2 is needed.

> [...]

> [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?

Yes, pre-existing. I can send a separate patch for this.