Re: [PATCH net v3 4/6] net: stmmac: selftests: Capture all packets for vlan checks

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

On 9/15/26 10:45, Paolo Abeni wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: stmmac: selftests: Capture all packets for vlan checks
> 
> The VLAN selftests use vlan_vid_add() without a VLAN netdev, so the test
> frame is marked PACKET_OTHERHOST and reaches the handler with a cleared
> skb->vlan_tci.  This patch reuses the DSA proxy mechanism to register an
> ETH_P_ALL handler for the three VLAN tests so validation sees the tag.
> 
> A few questions about the new capture_all path below.
> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
>> index 7f5837d1fed94..315c3cf82230e 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
>> @@ -242,6 +242,7 @@ struct stmmac_test_priv {
>>  	__be16 packet_type;
>>  	int (*func)(struct sk_buff *skb, struct net_device *ndev,
>>  		    struct packet_type *pt, struct net_device *orig_ndev);
>> +	bool capture_all;
>>  	int double_vlan;
>>  	int vlan_id;Hi,
>>  	int ok;
>> @@ -344,7 +345,7 @@ static void stmmac_sft_add_pack(struct packet_type *pt)
>>  {
>>  	struct stmmac_test_priv *tpriv = pt->af_packet_priv;
>>  
>> -	if (netdev_uses_dsa(tpriv->pt.dev)) {
>> +	if (netdev_uses_dsa(tpriv->pt.dev) || tpriv->capture_all) {
>>  		tpriv->packet_type = tpriv->pt.type;
>>  		tpriv->func = tpriv->pt.func;
>>  
>>  		/* DSA conduit will report ETH_P_XDSA, so our packet handler
>>  		 * won't match. Let's register a ETH_P_ALL match and filter
>>  		 * manually in stmmac_sft_filter.
>>  		 */
> 
> [Severity: Low]
> This isn't a bug, but the comment now only describes one of the two reasons
> for taking this branch.  For the three VLAN tests pt.type is ETH_P_IP or
> ETH_P_8021Q, which would match at ptype_specific just fine, so the
> "DSA conduit will report ETH_P_XDSA" rationale does not apply there.
> 
> Could the comment also mention the capture_all reason (the tap has to run
> at ptype_all so skb->vlan_tci is still populated), and could the new
> capture_all field get a one-line comment stating that it must be set before
> stmmac_sft_add_pack() is called?
> 
>>  		tpriv->pt.type = htons(ETH_P_ALL);
>>  		tpriv->pt.func = stmmac_sft_filter;
>>  		tpriv->pt.ignore_outgoing = true;
>>  	}
> 
> [Severity: Medium]
> With pt.type set to ETH_P_ALL on priv->dev, stmmac_test_vlan_validate() now
> sees every ingress frame, and it declares the test failed before it has
> established that the frame is a test frame:
> 
> 	if (tpriv->vlan_id) {
> 		if (skb->vlan_proto != htons(proto))
> 			goto out;
> 		if (skb->vlan_tci != tpriv->vlan_id) {
> 			/* Means filter did not work. */
> 			tpriv->ok = false;
> 			complete(&tpriv->comp);
> 			goto out;
> 		}
> 	}
> 
> 	ehdr = (struct ethhdr *)skb_mac_header(skb);
> 	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> 		goto out;
> 
> The h_dest and magic checks come after this.  Can any ordinary C-tagged
> IPv4 frame on the link (any VID other than 0x123, at least 33 bytes of
> linear data) now abort the VLAN Filter, Double VLAN Filter and VLAN Offload
> tests with a spurious failure for i == 0?  Before this patch the tag was
> cleared for such frames, so the vlan_proto check bailed out first.
> 
> Would it make sense to move the vlan_tci comparison after the destination
> MAC and magic checks so only test frames can set the verdict?

The ethtool selftests as they currently exists are very dependent on external
factors :
 - How was the link negociated (pause, eee)
 - Incoming traffic

So yes, ingress traffic may cause the selftest to fail :( I can move that check
around though for V4 :)

> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but the ETH_P_ALL tap widens the
> set of frames that reach the header parsing in stmmac_test_vlan_validate(),
> which derives offsets from the untrusted IPv4 ihl with only a fixed length
> check:
> 
> 	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
> 		goto out;
> 	...
> 	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
> 	if (uhdr->dest != htons(tpriv->packet->dport))
> 		goto out;
> 
> 	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
> 
> That check covers 33 bytes, which does not include the UDP header, the
> extra +4 double-VLAN offset, or an ihl larger than 5.  A frame with ihl = 15
> makes uhdr->dest be read well past the validated area (and past skb->len for
> a small frame).  The same arithmetic exists in
> stmmac_test_loopback_validate(), so this predates the patch, but would a
> pskb_may_pull() style length check based on the actual ihl be worth adding
> here?
> 
>> @@ -999,6 +1000,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
>>  	tpriv->pt.dev = priv->dev;
>>  	tpriv->pt.af_packet_priv = tpriv;
>>  	tpriv->packet = &attr;
>> +	tpriv->capture_all = true;
> 
> [Severity: Medium]
> This is a pre-existing issue, but the patch makes it observable.  The
> "Means filter did not work" path in stmmac_test_vlan_validate() reports
> failure through the same tpriv->ok = false state that a timeout produces,
> and the loop here treats that state as the expected outcome for i > 0:
> 
> 	wait_for_completion_timeout(&tpriv->comp, STMMAC_LB_TIMEOUT);
> 	ret = tpriv->ok ? 0 : -ETIMEDOUT;
> 	if (ret && !i) {
> 		goto vlan_del;
> 	} else if (!ret && i) {
> 		ret = -EINVAL;
> 		goto vlan_del;
> 	} else {
> 		ret = 0;
> 	}
> 
> So a frame admitted with a non-configured VID (0x124..0x126) is
> indistinguishable from a correctly dropped frame, and the test still passes
> even if the hardware admits every VID.  Before this patch the mismatch
> branch was unreachable because skb->vlan_tci was always cleared; now it is
> reachable but its verdict is discarded.  Could the return value of
> wait_for_completion_timeout() be checked, or a separate "packet was
> admitted" flag be used, so the two cases can be told apart?  The same
> pattern is in __stmmac_test_dvlanfilt().
> 
> [Severity: Medium]
> Also pre-existing rather than introduced here: tpriv->ok, tpriv->comp and
> the on-stack attr are shared between the softirq validator and this loop
> with no serialization, and init_completion() is only called once, outside
> the loop.  With capture_all, an extra complete() is now reachable from the
> "filter did not work" path for any tagged frame with a different VID.
> 
> Since complete() increments comp.done and wait_for_completion_timeout()
> consumes only one, does a surplus completion in one iteration make the next
> wait_for_completion_timeout() return immediately and take the verdict from a
> stale tpriv->ok?  Would a reinit_completion() at the top of each iteration
> help here?
> 
>> @@ -1093,6 +1095,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
>>  	tpriv->pt.dev = priv->dev;
>>  	tpriv->pt.af_packet_priv = tpriv;
>>  	tpriv->packet = &attr;
>> +	tpriv->capture_all = true;
> 
> [Severity: High]
> Does this actually fix the Double VLAN Filter test on the cores that can run
> it?  The test requires priv->dma_cap.vlhash, i.e. the xmac cores, where
> probe sets priv->hw->hw_vlan_en = true and stmmac_hw_setup() programs
> VLAN_TAG_STRIP_ALL together with VLAN_TAG_CTRL_EVLRXS, while
> vlan_vid_add(ETH_P_8021AD, ...) enables S-VLAN recognition.
> 
> The MAC-stripped outer tag is then reported by vlan_rx_hw() in
> stmmac_vlan.c:
> 
> 	__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
> 
> which hardcodes ETH_P_8021Q, while the validator with double_vlan set wants
> ETH_P_8021AD:
> 
> 	proto = tpriv->double_vlan ? ETH_P_8021AD : ETH_P_8021Q;
> 	...
> 		if (skb->vlan_proto != htons(proto))
> 			goto out;
> 
> So the frame does reach the validator now, but the vlan_proto check takes
> "goto out" without calling complete(), and the test still returns -ETIMEDOUT
> for i == 0 on working hardware unless RX VLAN stripping is turned off.  Is
> the intent that vlan_rx_hw() report the real tag protocol, or that the
> validator accept ETH_P_8021Q for the stripped outer tag?

Heh so since I sent this series, I've received more hardware, including
an dwxgmac2 platform ( finally \o/ )

And yeah this test fails on dwxgmac2 even with this patch :(

I'll address that in V4


> 
>> @@ -1371,6 +1374,7 @@ static int stmmac_test_vlanoff_common(struct stmmac_priv *priv, bool svlan)
>>  	tpriv->pt.af_packet_priv = tpriv;
>>  	tpriv->packet = &attr;
>>  	tpriv->vlan_id = 0x123;
>> +	tpriv->capture_all = true;
>>  	stmmac_sft_add_pack(&tpriv->pt);
>>  
>>  	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
> 
> [Severity: High]
> Can this dereference a NULL pointer in softirq context?  attr is
> zero-initialized, so attr.dst is NULL, and the ETH_P_ALL handler is
> published by stmmac_sft_add_pack() before attr.dst is assigned:
> 
> 	tpriv->capture_all = true;
> 	stmmac_sft_add_pack(&tpriv->pt);
> 
> 	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
> 	if (ret)
> 		goto cleanup;
> 
> 	attr.dst = priv->dev->dev_addr;
> 
> vlan_vid_add() is not instantaneous here: it does
> pm_runtime_resume_and_get() and a register poll in vlan_write_filter().  A
> frame arriving in that window with vlan_proto ETH_P_8021Q and vlan_tci equal
> to the hard-coded 0x123 passes the tag checks and falls into:
> 
> 	ehdr = (struct ethhdr *)skb_mac_header(skb);
> 	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> 		goto out;
> 
> which reads 6 bytes from address 0.  stmmac_test_loopback_validate() guards
> the same comparison with "if (dst)", so the pointer does not look like an
> invariant.  The same ordering exists in __stmmac_test_vlanfilt() and
> __stmmac_test_dvlanfilt(), where attr.dst is only set inside the loop.
> Would assigning attr.dst before stmmac_sft_add_pack(), or adding the NULL
> check to this validator, close that window?

Good point, I'll send a V4 then,

Maxime