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