[PATCH net-next 0/3] net: use skb_share_check() in loopback selftest hooks

[PATCH net-next 0/3] net: use skb_share_check() in loopback selftest hooks

[Nicolai Buchwitz]

The loopback selftest hooks can get a shared skb when another handler
is bound to the device. Linearizing a non-linear frame then hits
BUG_ON(skb_shared()) in pskb_expand_head().

Stumbled over this on RPi CM4 (bcmgenet, with jumbo series applied +
WIP selftest patches) with a packet socket open at the same time as
running ethtool -t.

stmmac and mlx5 seem to have the same pattern, xgbe already handles
shared skbs. Patches 2 and 3 are compile tested only.

As the impact is pretty limited and doesn't seem to bother anyone in
the wild, I've chosen net-next, but kept the Fixes tags. That matches
what Paolo suggested during the LPC track.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Nicolai Buchwitz (3):
      net: selftests: use skb_share_check() in the loopback receive hook
      net: stmmac: selftests: use skb_share_check() in the loopback receive hook
      net/mlx5e: use skb_share_check() in the loopback selftest hook

 drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c  | 4 ++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 +-
 net/core/selftests.c                                   | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)
---
base-commit: 45ad84d2800e4a092fb8d96006a533b2d0ab13f6
change-id: 20261007-nb-net-selftest-shared-net-823dc91ed5bf

Best regards,
--  
Nicolai Buchwitz <nb@tipi-net.de>

[PATCH net-next 1/3] net: selftests: use skb_share_check() in the loopback receive hook

[Nicolai Buchwitz]

The loopback hook gets a shared skb when another ETH_P_IP handler is
bound to the device. skb_unshare() does not unshare it, so
skb_linearize() on a non-linear frame hits BUG_ON(skb_shared()).

Use skb_share_check() instead.

Fixes: 3e1e58d64c3d ("net: add generic selftest support")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 net/core/selftests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/selftests.c b/net/core/selftests.c
index 36b949ae520b..80029a67b2d2 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -170,7 +170,7 @@ static int net_test_loopback_validate(struct sk_buff *skb,
 	struct tcphdr *thdr;
 	struct iphdr *ihdr;
 
-	skb = skb_unshare(skb, GFP_ATOMIC);
+	skb = skb_share_check(skb, GFP_ATOMIC);
 	if (!skb)
 		goto out;
 

-- 
2.53.0

[PATCH net-next 2/3] net: stmmac: selftests: use skb_share_check() in the loopback receive hook

[Nicolai Buchwitz]

The loopback hook gets a shared skb when another ETH_P_IP handler is
bound to the device. skb_unshare() does not unshare it, so
skb_linearize() on a non-linear frame hits BUG_ON(skb_shared()).

Use skb_share_check() instead.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 6097f312fce4..c910675a9d9b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -244,7 +244,7 @@ static int stmmac_test_loopback_validate(struct sk_buff *skb,
 	struct tcphdr *thdr;
 	struct iphdr *ihdr;
 
-	skb = skb_unshare(skb, GFP_ATOMIC);
+	skb = skb_share_check(skb, GFP_ATOMIC);
 	if (!skb)
 		goto out;
 

-- 
2.53.0

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

[Nicolai Buchwitz]

The loopback hook gets a shared skb when another ETH_P_IP handler is
bound to the device. skb_linearize() on a non-linear frame then hits
BUG_ON(skb_shared()).

Call skb_share_check() before linearizing the skb.

Fixes: 95b9606b15bb ("net/mlx5: Extend Ethtool loopback selftest to support non-linear SKB")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c b/drivers/net/ethernet/mellanox/mlx5/core/en_selftest.c
index 1dcdb86690bb..8eb9a9469549 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;
 

-- 
2.53.0

Re: [PATCH net-next 1/3] net: selftests: use skb_share_check() in the loopback receive hook

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The series fixes only part of this bug class.

Pre-existing issues:
- [Low] Pre-existing weak length validation in
  net_test_loopback_validate() (net/core/selftests.c).

--

net: selftests: use skb_share_check() in the loopback receive hook

This patch replaces skb_unshare() with skb_share_check() in
net_test_loopback_validate(). With the change, a shared but non-cloned skb
is cloned before skb_linearize() runs. Without it, a non-linear frame that
reaches the hook while another ETH_P_IP handler is bound to the device can
hit BUG_ON(skb_shared()).

> diff --git a/net/core/selftests.c b/net/core/selftests.c
> index 36b949ae520b3..80029a67b2d2b 100644
> --- a/net/core/selftests.c
> +++ b/net/core/selftests.c
> @@ -170,7 +170,7 @@ static int net_test_loopback_validate(struct sk_buff *skb,
>  	struct tcphdr *thdr;
>  	struct iphdr *ihdr;
>  
> -	skb = skb_unshare(skb, GFP_ATOMIC);
> +	skb = skb_share_check(skb, GFP_ATOMIC);
>  	if (!skb)
>  		goto out;
>  

[Severity: High]
This change looks correct for net_test_loopback_validate(). The same
pattern still seems to be present in stmmac_test_vlan_validate() in
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c, though.

A later commit in this series, "net: stmmac: selftests: use
skb_share_check() in the loopback receive hook", converts
stmmac_test_loopback_validate(). The second copy of this handler in the
same file is left as:

stmmac_test_vlan_validate() {
    ...
	skb = skb_unshare(skb, GFP_ATOMIC);
	if (!skb)
		goto out;

	if (skb_linearize(skb))
		goto out;
    ...
}

__stmmac_test_vlanfilt() and the other VLAN tests register this handler as
a device-bound packet_type. The type is ETH_P_IP, ETH_P_8021Q, or either
one depending on svlan:

	tpriv->pt.type = htons(ETH_P_IP);
	tpriv->pt.func = stmmac_test_vlan_validate;
	tpriv->pt.dev = priv->dev;

Suppose another handler for the same type sits later in the device's
ptype list, for example an AF_PACKET socket bound to ETH_P_IP on that
interface. deliver_skb() then increments skb->users, so the hook gets an
skb that is shared but not cloned. skb_unshare() only checks
skb_cloned(), so it returns that skb unchanged.

For a non-linear frame with less tailroom than data_len, this path
follows:

skb_linearize()
  __pskb_pull_tail()
    pskb_expand_head()
      BUG_ON(skb_shared(skb));

Should stmmac_test_vlan_validate() get the same skb_share_check()
conversion, so the series covers every copy of this hook?

The xgbe sibling, xgbe_test_loopback_validate(), does not seem to be
affected. It already calls skb_share_check() before it expands a shared
skb.

[Severity: Low]
This is a pre-existing issue that this patch did not introduce. The
length check further down in net_test_loopback_validate() looks too
short for what the function parses afterwards:

	if (skb_headlen(skb) < (NET_TEST_PKT_SIZE - ETH_HLEN))
		goto out;

NET_TEST_PKT_SIZE in include/net/selftests.h covers only ethhdr, iphdr
and netsfhdr. That guarantees 33 bytes past the network header and does
not include the TCP/UDP header.

The function then reads:

	thdr = (struct tcphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
	...
	if (shdr->magic != cpu_to_be64(NET_TEST_PKT_MAGIC))

It never checks ihl >= 5, the IP version, tot_len or the L4 header
length, and double_vlan adds another 4 bytes.

Could an IPv4 frame that arrives during ethtool -t make the hook read up
to about 64 bytes past skb->tail? Examples would be a minimum-size TCP
segment or a frame with ihl=15.

The over-read seems to stay inside the skb head allocation, because
skb_shared_info follows skb->end. If so, the only effect is a possibly
wrong selftest result.

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

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

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

Re: [PATCH net-next 1/3] net: selftests: use skb_share_check() in the loopback receive hook

[Nicolai Buchwitz]

Hi Clashiko

On 8.10.2026 23:31, netdev-bot+sashiko@kernel.org wrote:

> [...]

> [Severity: High]
> This change looks correct for net_test_loopback_validate(). The same
> pattern still seems to be present in stmmac_test_vlan_validate() in
> drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c, though.
> 
> A later commit in this series, "net: stmmac: selftests: use
> skb_share_check() in the loopback receive hook", converts
> stmmac_test_loopback_validate(). The second copy of this handler in the
> same file is left as:
> 
> stmmac_test_vlan_validate() {
>     ...
> 	skb = skb_unshare(skb, GFP_ATOMIC);
> 	if (!skb)
> 		goto out;
> 
> 	if (skb_linearize(skb))
> 		goto out;
>     ...
> }
> 
> __stmmac_test_vlanfilt() and the other VLAN tests register this handler 
> as
> a device-bound packet_type. The type is ETH_P_IP, ETH_P_8021Q, or 
> either
> one depending on svlan:
> 
> 	tpriv->pt.type = htons(ETH_P_IP);
> 	tpriv->pt.func = stmmac_test_vlan_validate;
> 	tpriv->pt.dev = priv->dev;
> 
> Suppose another handler for the same type sits later in the device's
> ptype list, for example an AF_PACKET socket bound to ETH_P_IP on that
> interface. deliver_skb() then increments skb->users, so the hook gets 
> an
> skb that is shared but not cloned. skb_unshare() only checks
> skb_cloned(), so it returns that skb unchanged.
> 
> For a non-linear frame with less tailroom than data_len, this path
> follows:
> 
> skb_linearize()
>   __pskb_pull_tail()
>     pskb_expand_head()
>       BUG_ON(skb_shared(skb));
> 
> Should stmmac_test_vlan_validate() get the same skb_share_check()
> conversion, so the series covers every copy of this hook?

No, AFAIU the VLAN tests can't get a shared skb here. They set
capture_all, so stmmac_sft_add_pack() registers stmmac_sft_filter()
instead, which passes stmmac_test_vlan_validate() its own clone.

> 
> The xgbe sibling, xgbe_test_loopback_validate(), does not seem to be
> affected. It already calls skb_share_check() before it expands a shared
> skb.
> 
> [Severity: Low]
> This is a pre-existing issue that this patch did not introduce. The
> length check further down in net_test_loopback_validate() looks too
> short for what the function parses afterwards:
> 
> 	if (skb_headlen(skb) < (NET_TEST_PKT_SIZE - ETH_HLEN))
> 		goto out;
> 
> NET_TEST_PKT_SIZE in include/net/selftests.h covers only ethhdr, iphdr
> and netsfhdr. That guarantees 33 bytes past the network header and does
> not include the TCP/UDP header.
> 
> The function then reads:
> 
> 	thdr = (struct tcphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
> 	...
> 	if (shdr->magic != cpu_to_be64(NET_TEST_PKT_MAGIC))
> 
> It never checks ihl >= 5, the IP version, tot_len or the L4 header
> length, and double_vlan adds another 4 bytes.
> 
> Could an IPv4 frame that arrives during ethtool -t make the hook read 
> up
> to about 64 bytes past skb->tail? Examples would be a minimum-size TCP
> segment or a frame with ihl=15.
> 
> The over-read seems to stay inside the skb head allocation, because
> skb_shared_info follows skb->end. If so, the only effect is a possibly
> wrong selftest result.

The length check is pre-existing, will address that in a follow-up 
patch.

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