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

On 8.10.2026 23:31, netdev-bot+sashiko@kernel.org 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.