[PATCH net 0/4] net: dsa: motorcomm: ACL fixup

[PATCH net 0/4] net: dsa: motorcomm: ACL fixup

[David Yang]

Various ACL defects. Found by LLM and manually verified (on YT9215).

David Yang (4):
  net: dsa: motorcomm: Fix extack message on unsupported TC actions
  net: dsa: motorcomm: Fix IPv4/IPv6 address key dispatch in flower
  net: dsa: motorcomm: Stop at the first terminal flower action
  net: dsa: motorcomm: Delete the ACL rule of the port being unbound

 drivers/net/dsa/motorcomm/tc.c | 87 ++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 42 deletions(-)

-- 
2.53.0

[PATCH net 1/4] net: dsa: motorcomm: Fix extack message on unsupported TC actions

[David Yang]

No extack reasons were given to unsupported TC actions. Fix it.

Fixes: 8eb9e7520b17 ("net: dsa: yt921x: Add ACL support")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/tc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
index 9dbd61e48fee..8835907366bf 100644
--- a/drivers/net/dsa/motorcomm/tc.c
+++ b/drivers/net/dsa/motorcomm/tc.c
@@ -1122,6 +1122,7 @@ yt921x_acl_rule_ext_parse_flow_action(struct yt921x_acl_rule_ext *ruleext,
 			break;
 		}
 		default:
+			reason = "Unsupported by HW";
 fallback:
 			if (cls->common.skip_sw) {
 				NL_SET_ERR_MSG_FMT_MOD(extack,
-- 
2.53.0

[PATCH net 2/4] net: dsa: motorcomm: Fix IPv4/IPv6 address key dispatch in flower

[David Yang]

The IPv4 and IPv6 address keys share the same storage in struct
flow_rule, so flow_rule_match_key() reports both as present whenever
either one is. As a result, IPv4 address rule may generated bogus IPv6
address bins, and vice versa.

Dispatch on control.addr_type like other DSA drivers do.

Fixes: 8eb9e7520b17 ("net: dsa: yt921x: Add ACL support")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/tc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
index 8835907366bf..5a4459507582 100644
--- a/drivers/net/dsa/motorcomm/tc.c
+++ b/drivers/net/dsa/motorcomm/tc.c
@@ -580,6 +580,7 @@ yt921x_acl_rule_ext_parse_flow_entries(struct yt921x_acl_rule_ext *ruleext,
 	unsigned int udfs_cnt = 0;
 	unsigned int size = 0;
 	__be16 n_proto = 0;
+	u16 addr_type = 0;
 	u8 ip_proto = 0;
 	bool use_dport;
 	bool use_sport;
@@ -603,8 +604,15 @@ yt921x_acl_rule_ext_parse_flow_entries(struct yt921x_acl_rule_ext *ruleext,
 		return -EOPNOTSUPP;
 	}
 
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) {
+		struct flow_match_control match;
+
+		flow_rule_match_control(rule, &match);
+		addr_type = match.key->addr_type;
+	}
+
 	/* Entries */
-	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS)) {
+	if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
 		struct flow_match_ipv4_addrs match;
 
 		flow_rule_match_ipv4_addrs(rule, &match);
@@ -630,7 +638,7 @@ yt921x_acl_rule_ext_parse_flow_entries(struct yt921x_acl_rule_ext *ruleext,
 		}
 	}
 
-	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV6_ADDRS)) {
+	if (addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS) {
 		struct flow_match_ipv6_addrs match;
 
 		flow_rule_match_ipv6_addrs(rule, &match);
-- 
2.53.0

[PATCH net 3/4] net: dsa: motorcomm: Stop at the first terminal flower action

[David Yang]

tc software stops evaluating a flower action list at the first terminal
action; the hardware translation did not, so a later trap or redirect
silently overwrote the verdict of the actions before it: an `action drop
action trap` filter with skip_sw trapped packets to the CPU instead of
dropping them, and repeated redirects to different ports accumulated
every target in hardware while software only honored the first one.

Fix it by resolving only the first terminal action of the list.

Fixes: 8eb9e7520b17 ("net: dsa: yt921x: Add ACL support")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/tc.c | 61 +++++++++++++++-------------------
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
index 5a4459507582..87c04f0dd736 100644
--- a/drivers/net/dsa/motorcomm/tc.c
+++ b/drivers/net/dsa/motorcomm/tc.c
@@ -1038,55 +1038,43 @@ yt921x_acl_rule_ext_parse_flow_action(struct yt921x_acl_rule_ext *ruleext,
 	const struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
 	const struct flow_action *flow_action = &rule->action;
 	struct netlink_ext_ack *extack = cls->common.extack;
-	enum flow_action_id redir_act = NUM_FLOW_ACTIONS;
 	const struct flow_action_entry *act;
 	u32 *action = ruleext->r.action;
 	bool seen_priority = false;
 	const char *reason = NULL;
 	bool seen_police = false;
+	bool terminal = false;
 	unsigned int i;
 	int res;
 
 	memset(action, 0, 3 * sizeof(*action));
-	flow_action_for_each(i, act, flow_action)
+	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_ACCEPT:
+			action[2] |= YT921X_ACL_ACTc_FWD_EN |
+				     YT921X_ACL_ACTc_FWD_FWD;
+			terminal = true;
+			break;
 		case FLOW_ACTION_DROP:
-		case FLOW_ACTION_REDIRECT:
-			if (redir_act != NUM_FLOW_ACTIONS &&
-			    redir_act != act->id) {
-				reason = "Different redirect actions";
+			action[2] |= YT921X_ACL_ACTc_FWD_EN |
+				     YT921X_ACL_ACTc_FWD_REDIR;
+			terminal = true;
+			break;
+		case FLOW_ACTION_REDIRECT: {
+			struct dsa_port *to_dp;
+
+			to_dp = dsa_port_from_netdev(act->dev);
+			if (IS_ERR(to_dp) || to_dp->ds != &priv->ds) {
+				reason = "Redirect to non-local port";
 				goto fallback;
 			}
-			redir_act = act->id;
-
-			switch (act->id) {
-			case FLOW_ACTION_ACCEPT:
-				action[2] |= YT921X_ACL_ACTc_FWD_EN |
-					     YT921X_ACL_ACTc_FWD_FWD;
-				break;
-			case FLOW_ACTION_DROP:
-				action[2] |= YT921X_ACL_ACTc_FWD_EN |
-					     YT921X_ACL_ACTc_FWD_REDIR;
-				break;
-			case FLOW_ACTION_REDIRECT: {
-				struct dsa_port *to_dp;
-
-				to_dp = dsa_port_from_netdev(act->dev);
-				if (IS_ERR(to_dp) || to_dp->ds != &priv->ds) {
-					reason = "Redirect to non-local port";
-					goto fallback;
-				}
 
-				action[2] |= YT921X_ACL_ACTc_FWD_EN |
-					     YT921X_ACL_ACTc_FWD_REDIR |
-					     YT921X_ACL_ACTc_FWD_REDIR_DPORTn(to_dp->index);
-				break;
-			}
-			default:
-				break;
-			}
+			action[2] |= YT921X_ACL_ACTc_FWD_EN |
+				     YT921X_ACL_ACTc_FWD_REDIR |
+				     YT921X_ACL_ACTc_FWD_REDIR_DPORTn(to_dp->index);
+			terminal = true;
 			break;
+		}
 		case FLOW_ACTION_PRIORITY:
 			if (seen_priority) {
 				action[0] &= ~YT921X_ACL_ACTa_PRIO_EN;
@@ -1140,15 +1128,18 @@ yt921x_acl_rule_ext_parse_flow_action(struct yt921x_acl_rule_ext *ruleext,
 			}
 			fallthrough;
 		case FLOW_ACTION_TRAP:
-			redir_act = FLOW_ACTION_TRAP;
-
 			action[2] &= ~YT921X_ACL_ACTc_FWD_REDIR_DPORTS_M &
 				     ~YT921X_ACL_ACTc_FWD_M;
 			action[2] |= YT921X_ACL_ACTc_FWD_EN |
 				     YT921X_ACL_ACTc_FWD_TRAP;
+			terminal = true;
 			break;
 		}
 
+		if (terminal)
+			break;
+	}
+
 	ruleext->r.sw_assisted = !cls->common.skip_sw;
 	return 0;
 }
-- 
2.53.0

[PATCH net 4/4] net: dsa: motorcomm: Delete the ACL rule of the port being unbound

[David Yang]

A flower filter on a block shared across ports is offloaded as one
hardware rule per port, all carrying the same classifier cookie.
Deletion looked a rule up by cookie alone and could take another
port's copy: with skip_sw, a filter silently stopped being enforced
on a still-bound port, while the detached port kept enforcing it.

Delete the rule matching the port being unbound instead.

Fixes: 8eb9e7520b17 ("net: dsa: yt921x: Add ACL support")
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/tc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
index 87c04f0dd736..235ecc1eedc9 100644
--- a/drivers/net/dsa/motorcomm/tc.c
+++ b/drivers/net/dsa/motorcomm/tc.c
@@ -1223,7 +1223,7 @@ static u16 yt921x_acl_activated_ports(const struct yt921x_priv *priv)
 
 static unsigned int
 yt921x_acl_find(const struct yt921x_priv *priv, enum tc_setup_type type,
-		unsigned long tag)
+		unsigned long tag, u16 ports_mask)
 {
 	for (unsigned int blkid = 0; blkid < YT921X_ACL_BLK_NUM; blkid++) {
 		const struct yt921x_acl_blk *aclblk = priv->acl_blks[blkid];
@@ -1233,7 +1233,10 @@ yt921x_acl_find(const struct yt921x_priv *priv, enum tc_setup_type type,
 
 		for (unsigned int i = 0; i < YT921X_ACL_ENT_PER_BLK; i++)
 			if (aclblk->rules[i] && aclblk->rules[i]->tag == tag &&
-			    aclblk->rules[i]->type == type)
+			    aclblk->rules[i]->type == type &&
+			    FIELD_GET(YT921X_ACL_KEYb_SPORTS_M,
+				      aclblk->rules[i]->entries[0].key[1]) ==
+			    ports_mask)
 				return YT921X_ACL_ENT_PER_BLK * blkid + i;
 	}
 
@@ -1361,7 +1364,7 @@ yt921x_acl_commit(struct yt921x_priv *priv, unsigned int entid, u8 entsmask)
 
 static int
 yt921x_acl_del(struct yt921x_priv *priv, enum tc_setup_type type,
-	       unsigned long tag)
+	       unsigned long tag, u16 ports_mask)
 {
 	struct yt921x_acl_rule *aclrule;
 	struct yt921x_acl_blk *aclblk;
@@ -1375,7 +1378,7 @@ yt921x_acl_del(struct yt921x_priv *priv, enum tc_setup_type type,
 	int res;
 	int ret;
 
-	entid = yt921x_acl_find(priv, type, tag);
+	entid = yt921x_acl_find(priv, type, tag, ports_mask);
 	if (entid == UINT_MAX)
 		return -ENOENT;
 
@@ -1550,7 +1553,7 @@ yt921x_dsa_cls_flower_del(struct dsa_switch *ds, int port,
 	int res;
 
 	mutex_lock(&priv->reg_lock);
-	res = yt921x_acl_del(priv, TC_SETUP_CLSFLOWER, cls->cookie);
+	res = yt921x_acl_del(priv, TC_SETUP_CLSFLOWER, cls->cookie, BIT(port));
 	mutex_unlock(&priv->reg_lock);
 
 	return res;
-- 
2.53.0

Re: [PATCH net 2/4] net: dsa: motorcomm: Fix IPv4/IPv6 address key dispatch in flower

[netdev-bot+sashiko]

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

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

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 commit replaces the used_keys presence checks with a
  control.addr_type check, and this introduces a regression.
- [Medium] The commit message describes a bug that does not exist in this
  tree.

--

net: dsa: motorcomm: Fix IPv4/IPv6 address key dispatch in flower

This patch changes yt921x_acl_rule_ext_parse_flow_entries() so that it
chooses the IPv4 or IPv6 address block from control.addr_type. Before,
it checked whether the matching address key was set in used_keys. The
commit message says flow_rule_match_key() reports both address keys
when only one of them is used.

> The IPv4 and IPv6 address keys share the same storage in struct
> flow_rule, so flow_rule_match_key() reports both as present whenever
> either one is. As a result, IPv4 address rule may generated bogus IPv6
> address bins, and vice versa.

[Severity: Medium]
Is this description accurate? flow_rule_match_key() is just
dissector_uses_key(), which tests one bit in used_keys:

include/net/flow_dissector.h:dissector_uses_key() {
	return flow_dissector->used_keys & (1ULL << key_id);
}

FLOW_DISSECTOR_KEY_IPV4_ADDRS and FLOW_DISSECTOR_KEY_IPV6_ADDRS are
separate bits.

In flower, struct fl_flow_key has separate ipv4 and ipv6 members (not a
union). fl_set_key() fills in only one family. fl_init_dissector()
registers each key on its own, based on that key's own mask:

net/sched/cls_flower.c:fl_init_dissector() {
    ...
	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
			     FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
			     FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);
    ...
}

fl_hw_replace_filter() then passes that dissector to the driver
unchanged.

In nftables offload, nft_flow_key does put ipv4 and ipv6 in a union.
However, nft_payload_offload_ip() and nft_payload_offload_ip6() only
set the bit for the family being matched.

Is there a path where both bits end up set? If not, the old code looks
correct for these inputs. The patch has a Fixes: tag and will be
backported, so could the commit message be corrected, or could it
include a reproducer?

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> index 8835907366bf7..5a4459507582d 100644
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c

[ ... ]

> @@ -603,8 +604,15 @@ yt921x_acl_rule_ext_parse_flow_entries(struct yt921x_acl_rule_ext *ruleext,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) {
> +		struct flow_match_control match;
> +
> +		flow_rule_match_control(rule, &match);
> +		addr_type = match.key->addr_type;
> +	}
> +
>  	/* Entries */
> -	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS)) {
> +	if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
>  		struct flow_match_ipv4_addrs match;
>  
>  		flow_rule_match_ipv4_addrs(rule, &match);
> @@ -630,7 +638,7 @@ yt921x_acl_rule_ext_parse_flow_entries(struct yt921x_acl_rule_ext *ruleext,
>  		}
>  	}
>  
> -	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV6_ADDRS)) {
> +	if (addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS) {
>  		struct flow_match_ipv6_addrs match;
>  
>  		flow_rule_match_ipv6_addrs(rule, &match);

[Severity: High]
Can this program bogus IPv6 bins when the IPv6 address mask is all
zero?

fl_set_key() sets addr_type whenever an IPv6 src or dst attribute is
present, whatever the mask is:

	} else if (tb[TCA_FLOWER_KEY_IPV6_SRC] || tb[TCA_FLOWER_KEY_IPV6_DST]) {
		key->control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
		mask->control.addr_type = ~0;

fl_init_dissector() only registers FLOW_DISSECTOR_KEY_IPV6_ADDRS through
FL_KEY_SET_IF_MASKED. Take a rule like this:

  tc filter add dev swp0 ingress protocol ipv6 flower skip_sw \
      src_ip ::/0 action drop

It ends up with addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS, but with no
IPV6_ADDRS bit in used_keys.

skb_flow_dissector_init() memsets the dissector, so
offset[FLOW_DISSECTOR_KEY_IPV6_ADDRS] stays 0. skb_flow_dissector_target()
adds that offset without checking whether the key is present. As a
result, flow_rule_match_ipv6_addrs() returns key and mask pointers to
the start of struct fl_flow_key (meta, control, enc_control, basic, ...).

With that layout, mask->src.s6_addr32[2] overlays control.thoff and
control.addr_type, and the addr_type mask is 0xffff. mask->dst.s6_addr32[2]
overlays basic.n_proto and basic.ip_proto, and the n_proto mask is 0xffff
when a protocol is given.

Wouldn't the loops then create YT921X_ACL_TYPE_IPV6_SA2 and
YT921X_ACL_TYPE_IPV6_DA2 entries keyed on addr_type and the 0x86dd
EtherType? The hardware rule would then match only IPv6 packets whose
address words happen to equal those values, not all IPv6 traffic.

Other matches such as ip_flags or dst_mac can add SA3/DA3 bins in the
same way. The extra bins can also push an otherwise valid rule into the
"Rule too complex" error path.

Before this patch, flow_rule_match_key(rule,
FLOW_DISSECTOR_KEY_IPV6_ADDRS) returned false here and the block was
skipped.

The IPv4 branch has the same structure. It is harmless today only
because offset 0 hits the meta key, and the meta mask is zero since META
is rejected in used_keys.

This also looks to be still present at the end of the series. The
commit "net: dsa: motorcomm: Delete the ACL rule of the port being
unbound" keeps the addr_type-only conditions.

Would it work to keep the presence check and add the addr_type check on
top of it? For example:

	if (addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS &&
	    flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV6_ADDRS)) {

and the same for the IPv4 branch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102071119.3435180-1-mmyangfl%40gmail.com

Re: [PATCH net 3/4] net: dsa: motorcomm: Stop at the first terminal flower action

[netdev-bot+sashiko]

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

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

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

- [Medium] In yt921x_acl_rule_ext_parse_flow_action()
  (drivers/net/dsa/motorcomm/tc.c), the patch now treats every…

--

net: dsa: motorcomm: Stop at the first terminal flower action

This commit changes yt921x_acl_rule_ext_parse_flow_action() so that it
stops at the first terminal action in the flower action list (accept,
drop, redirect or trap). A later action can then no longer override the
hardware verdict of an earlier one.

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> index 5a4459507582d..87c04f0dd7365 100644
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c
> @@ -1038,55 +1038,43 @@ yt921x_acl_rule_ext_parse_flow_action(struct yt921x_acl_rule_ext *ruleext,

[ ... ]

> +		case FLOW_ACTION_REDIRECT: {
> +			struct dsa_port *to_dp;
> +
> +			to_dp = dsa_port_from_netdev(act->dev);
> +			if (IS_ERR(to_dp) || to_dp->ds != &priv->ds) {
> +				reason = "Redirect to non-local port";
>  				goto fallback;
>  			}

[ ... ]

> +			action[2] |= YT921X_ACL_ACTc_FWD_EN |
> +				     YT921X_ACL_ACTc_FWD_REDIR |
> +				     YT921X_ACL_ACTc_FWD_REDIR_DPORTn(to_dp->index);
> +			terminal = true;
>  			break;
> +		}

[Severity: Medium]
Is it safe to treat every FLOW_ACTION_REDIRECT as terminal here?

The flow_action_entry does not carry the mirred control verdict. For any
egress redirect, tcf_mirred_offload_act_setup() emits FLOW_ACTION_REDIRECT
and does not look at m->tcf_action:

net/sched/act_mirred.c:tcf_mirred_offload_act_setup() {
    ...
		if (is_tcf_mirred_egress_redirect(act)) {
			entry->id = FLOW_ACTION_REDIRECT;
			tcf_offload_mirred_get_dev(entry, act);
    ...
}

A redirect with the pipe control is not terminal in software.
tcf_mirred_init() accepts TC_ACT_PIPE through tcf_action_check_ctrlact().
In tcf_mirred_to_dev(), tcf_mirred_can_reinsert(TC_ACT_PIPE) returns
false, so the skb is cloned and the clone is sent. TC_ACT_PIPE is then
returned and the remaining actions run.

For example:

  tc filter add dev swp0 ingress flower skip_sw \
    action mirred egress redirect dev swp1 pipe \
    action mirred egress redirect dev swp2

In software this delivers to both swp1 and swp2. Before this patch the
hardware ORed both DPORTn bits, which matched software. With this patch
the loop exits at the if (terminal) break after the first redirect. Only
swp1 gets programmed, and the rule is accepted with no error and no
extack warning.

The same applies to "redirect dev swp1 pipe action trap". In software
that sends a copy to swp1 and also traps to the CPU. In hardware it would
now only redirect to swp1.

The driver cannot tell stolen apart from pipe. Could a redirect followed
by more actions be rejected under skip_sw, and fall back to trap
otherwise, instead of programming only part of the list? That would
match the "Prefer explicit rejects over implicit behaviour changes"
policy in yt921x_acl_rule_ext_parse_flow(). In the default stolen case
the actions after the redirect never run in software anyway, so
rejecting them loses nothing.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102071119.3435180-1-mmyangfl%40gmail.com