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