Re: [PATCH net v3 1/6] net: stmmac: selftests: Support running selftests on DSA conduits

[Paolo Abeni <pabeni@redhat.com>]

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: Support running selftests on DSA conduits

The patch wraps the selftests' dev_add_pack()/dev_remove_pack() calls in
new stmmac_sft_add_pack()/stmmac_sft_remove_pack() helpers.  When the
interface is a DSA conduit, an ETH_P_ALL packet_type is registered instead
and a proxy handler, stmmac_sft_filter(), compares the ethernet header's
h_proto before forwarding a clone to the original validator.

A few questions below, one on the commit message and three on the code.

> The ARP offload's packet handler is left alone, this feature is just not
> implemented in stmmac and due for removal.

[Severity: Medium]
Is this description accurate?  ARP offload does look implemented in two
places:

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
	static void dwmac4_set_arp_offload(struct mac_device_info *hw, bool en,
					   u32 addr)

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
	static void dwxgmac2_set_arp_offload(struct mac_device_info *hw, bool en,
					     u32 addr)

dwmac4_set_arp_offload() is wired into dwmac4_ops, dwmac410_ops and
dwmac510_ops, and dwxgmac2_set_arp_offload() into dwxgmac210_ops and
dwxlgmac2_ops.

stmmac_test_arpoffload() only bails out early with:

	if (!priv->dma_cap.arpoffsel)
		return -EOPNOTSUPP;

so on those cores with arpoffsel set the test really runs, and it is still
listed in stmmac_selftests as "ARP Offload".  Since it keeps the raw
dev_add_pack(&tpriv->pt) with pt.type = htons(ETH_P_ARP), does it still
report a spurious failure in ethtool -t on a DSA conduit, for the same
ETH_P_XDSA reason this patch fixes elsewhere?  Would switching that pair to
stmmac_sft_add_pack()/stmmac_sft_remove_pack() cover it as well?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 6372ec7c3f317..6503678b646ef 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c

[ ... ]

> @@ -237,6 +238,9 @@ struct stmmac_test_priv {
>  	struct stmmac_packet_attrs *packet;
>  	struct packet_type pt;
>  	struct completion comp;
> +	__be16 packet_type;
> +	int (*func)(struct sk_buff *skb, struct net_device *ndev,
> +		    struct packet_type *pt, struct net_device *orig_ndev);
>  	int double_vlan;
>  	int vlan_id;
>  	int ok;

[ ... ]

> @@ -316,6 +320,50 @@ static int stmmac_test_loopback_validate(struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static int stmmac_sft_filter(struct sk_buff *skb, struct net_device *ndev,
> +			     struct packet_type *pt,
> +			     struct net_device *orig_ndev)
> +{
> +	struct stmmac_test_priv *tpriv = pt->af_packet_priv;
> +	struct ethhdr *hdr = eth_hdr(skb);
> +	int ret = 0;
> +
> +	if (hdr->h_proto == tpriv->packet_type) {
> +		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
> +
> +		if (nskb)
> +			ret = tpriv->func(nskb, ndev, pt, orig_ndev);
> +	}

[Severity: Medium]
This compares the on-wire ethertype against the value the test registered
in pt.type, but for the VLAN tests pt.type is the protocol *after* the tag
has been removed:

__stmmac_test_vlanfilt()      pt.type = htons(ETH_P_IP), frame is 802.1Q
__stmmac_test_dvlanfilt()     pt.type = htons(ETH_P_8021Q), outer tag is
                              ETH_P_8021AD
stmmac_test_vlanoff_common()  pt.type = ETH_P_IP or ETH_P_8021Q

On a normal interface the core removes the tag before the taps run:

net/core/dev.c:__netif_receive_skb_core() {
	...
	if (eth_type_vlan(skb->protocol)) {
		skb = skb_vlan_untag(skb);
	...
	list_for_each_entry_rcu(ptype, &skb->dev->ptype_all, list) {
	...
}

but on a DSA conduit skb->protocol is ETH_P_XDSA, so eth_type_vlan() is
false and no untagging happens there.  Doesn't that leave the driver as the
only tag remover, and isn't that conditional?

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
	if ((vlan_proto == htons(ETH_P_8021Q) &&
	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
	    (vlan_proto == htons(ETH_P_8021AD) &&
	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
	...
}

NETIF_F_HW_VLAN_CTAG_RX is placed in hw_features on xmac cores, so it is
user-toggleable.  After "ethtool -K <conduit> rxvlan off", or with
CONFIG_VLAN_8021Q disabled where neither feature nor hw_vlan_en is ever
set, the frame should reach the filter with h_proto == 0x8100, the
comparison against ETH_P_IP fails, the frame is freed here and the C-VLAN
tests time out.  Would matching on the inner protocol (or skipping any
VLAN tags before the comparison) be more robust?

> +
> +	kfree_skb(skb);
> +	return ret;
> +}

[Severity: Medium]
This isn't a bug introduced by this patch, but the validators reached from
here derive header offsets from the received IPv4 ihl field while only
checking for 33 linear bytes:

stmmac_test_loopback_validate() {
	...
	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
		goto out;
	...
	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
	...
	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
	...
	if (shdr->magic != cpu_to_be64(STMMAC_TEST_PKT_MAGIC))
	...
}

With ihl == 15 the shdr->magic read lands roughly 90 bytes past the checked
bound.  The same pattern exists in stmmac_test_vlan_validate().  This was
already reachable before the patch through the ETH_P_IP ptype registration
on priv->dev, and the read stays inside the RX buffer, so the practical
effect is stale bytes and possibly a wrong verdict.  Since the new
ETH_P_ALL tap widens the set of frames that get here, would it be
reasonable to add a length check on the derived offsets along with it?

> +
> +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)) {
> +		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.
> +		 */
> +		tpriv->pt.type = htons(ETH_P_ALL);
> +		tpriv->pt.func = stmmac_sft_filter;
> +		tpriv->pt.ignore_outgoing = true;
> +	}
> +
> +	dev_add_pack(pt);
> +}
> +
> +static void stmmac_sft_remove_pack(struct packet_type *pt)
> +{
> +	dev_remove_pack(pt);
> +}
> +
>  static int __stmmac_test_loopback(struct stmmac_priv *priv,
>  				  struct stmmac_packet_attrs *attr)
>  {

[ ... ]

> @@ -928,7 +976,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
>  	 * HASH values.
>  	 */
>  	tpriv->vlan_id = 0x123;
> -	dev_add_pack(&tpriv->pt);
> +	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
>  	if (ret)

[Severity: High]
Can an incoming frame from the switch oops here?  The handler is now
installed as an ETH_P_ALL tap while attr.dst is still NULL:

__stmmac_test_vlanfilt() {
	struct stmmac_packet_attrs attr = { };
	...
	tpriv->packet = &attr;
	tpriv->vlan_id = 0x123;
	stmmac_sft_add_pack(&tpriv->pt);

	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
	...
	for (i = 0; i < 4; i++) {
		...
		attr.dst = priv->dev->dev_addr;

and the VLAN validator dereferences packet->dst without a NULL check,
unlike stmmac_test_loopback_validate() which guards it with "if (dst)":

stmmac_test_vlan_validate() {
	...
	if (tpriv->vlan_id) {
		if (skb->vlan_proto != htons(proto))
			goto out;
		if (skb->vlan_tci != tpriv->vlan_id) {
	...
	ehdr = (struct ethhdr *)skb_mac_header(skb);
	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
	...
}

The ETH_P_ALL tap runs before the core's __vlan_hwaccel_clear_tag(), so a
received IPv4 frame that stmmac_rx_vlan() stripped into the hwaccel fields
with vlan_proto == ETH_P_8021Q and vlan_tci == 0x123 passes the h_proto
check in stmmac_sft_filter(), passes the vlan_proto/vlan_tci checks, and
then calls ether_addr_equal_unaligned(ehdr->h_dest, NULL).

Before the patch the core cleared the hwaccel tag before ETH_P_IP ptype
dispatch, so foreign frames were rejected by the vlan_proto check and this
window was not reachable from the wire.  Would setting attr.dst before
stmmac_sft_add_pack(), or adding a NULL guard in the validator, close it?

> @@ -1022,7 +1070,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
>  	 * HASH values.
>  	 */
>  	tpriv->vlan_id = 0x123;
> -	dev_add_pack(&tpriv->pt);
> +	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
>  	if (ret)

[ ... ]

> @@ -1293,7 +1341,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;
> -	dev_add_pack(&tpriv->pt);
> +	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
>  	if (ret)

[Severity: High]
Same ordering as __stmmac_test_vlanfilt() here: the tap is registered, then
vlan_vid_add() runs, and only afterwards does the function do
"attr.dst = priv->dev->dev_addr;", so tpriv->packet->dst is NULL for the
whole window.

[ ... ]
-- 
This is an AI-generated review.