Re: [PATCH net] net: stmmac: selftests: Pass the IP proto mask in the TC selftest

[Paolo Abeni]

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: Pass the IP proto mask in the TC selftest

This adds an ip_proto mask to the match data used by the L4 filtering
selftests in __stmmac_test_l4filt(), so that the new mask sanity checks in
tc_add_basic_flow() accept the rule.  The commit message reports tests 23-26
going from -22 to 0 on imx8mp.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 1df26c217f9a..e9be4d7cbb82 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> @@ -1489,6 +1489,8 @@ static int __stmmac_test_l4filt(struct stmmac_priv *priv, u32 dst, u32 src,
>  	keys.bkey.ip_proto = udp ? IPPROTO_UDP : IPPROTO_TCP;
>  	keys.key.src = htons(src);
>  	keys.key.dst = htons(dst);
> +	/* Match the full IP proto field */
> +	masks.bmask.ip_proto = 0xff;
>  	masks.mask.src = src_mask;
>  	masks.mask.dst = dst_mask;
>  

[Severity: Medium]

Is the n_proto half of the basic key still left uninitialized here?  In
__stmmac_test_l4filt() both structs are declared without an initializer:

	struct {
		struct flow_dissector_key_basic bkey;
		struct flow_dissector_key_ports key;
	} __aligned(BITS_PER_LONG / 8) keys;
	struct {
		struct flow_dissector_key_basic bmask;
		struct flow_dissector_key_ports mask;
	} __aligned(BITS_PER_LONG / 8) masks;

The function then only assigns keys.bkey.ip_proto, keys.key.src,
keys.key.dst, and now masks.bmask.ip_proto, masks.mask.src, masks.mask.dst.
Since dissector->offset[FLOW_DISSECTOR_KEY_BASIC] is 0,
flow_rule_match_basic() hands tc_add_basic_flow() pointers straight at
keys.bkey / masks.bmask, so match.mask->n_proto and match.key->n_proto are
whatever was on the stack.

tc_add_basic_flow() in stmmac_tc.c makes decisions on exactly those fields:

	if ((match.mask->n_proto && match.mask->n_proto != ETHER_TYPE_FULL_MASK) ||
	    (match.mask->ip_proto && match.mask->ip_proto != IP_PROTO_FULL_MASK)) {
		NL_SET_ERR_MSG_MOD(cls->common.extack,
				   "only full protocol mask is supported");
		return -EOPNOTSUPP;
	}

	/* Network proto is present in the key and is not IPv4 */
	if (match.mask->n_proto && match.key->n_proto != cpu_to_be16(ETH_P_IP)) {
		NL_SET_ERR_MSG_MOD(cls->common.extack,
				   "only IPv4 network protocol is supported");
		return -EOPNOTSUPP;
	}

So if the stack slot for masks.bmask.n_proto happens to be non-zero and not
0xffff, does the rule get rejected with -EOPNOTSUPP again?  With
CONFIG_INIT_STACK_ALL_PATTERN the slot holds the 0xAA pattern, which is
neither 0 nor ETHER_TYPE_FULL_MASK.

>From there tc_add_flow_cls() falls back:

	ret = tc_add_flow(priv, cls);
	if (!ret)
		return ret;

	ret = tc_add_ethtype_flow(priv, cls);
	if (!ret)
		return ret;

	return tc_add_vlan_flow(priv, cls);

and with cls->classid == 0 those fallbacks return -EINVAL, which is the same
-22 that tests 23-26 report in the commit message.  Would zero-initializing
both structs ("= { }", as attr already does in the same function), and
setting keys.bkey.n_proto = htons(ETH_P_IP) together with
masks.bmask.n_proto = ETHER_TYPE_FULL_MASK given the injected frames are
IPv4, make the result independent of the stack contents?
-- 
This is an AI-generated review.