Re: [PATCH net v2 2/3] net: dsa: motorcomm: Resolve the flower action list to one ACL verdict

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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 patch says it makes the ACL use one verdict, the first
  terminal action.

--

net: dsa: motorcomm: Resolve the flower action list to one ACL verdict

This commit changes yt921x_acl_rule_ext_parse_flow_action() to resolve
the flower action list against its first terminal action, so a later
action no longer overrides an earlier verdict. Consecutive redirects are
still merged into the destination port mask. Other actions that follow a
redirect are rejected under skip_sw, or trapped to the CPU otherwise.

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

[ ... ]

>  	memset(action, 0, 3 * sizeof(*action));
> -	flow_action_for_each(i, act, flow_action)
> +	flow_action_for_each(i, act, flow_action) {
> +		/* Only another redirect, or a drop, can follow a redirect: a
> +		 * drop is an empty destination set, so the software verdict is
> +		 * the same whichever control the redirect carries.
> +		 */
> +		if (redir && act->id != FLOW_ACTION_REDIRECT &&
> +		    act->id != FLOW_ACTION_DROP) {
> +			reason = "Action after redirect";
> +			goto fallback;
> +		}
> +

[ ... ]

> +		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);
> +			redir = true;
>  			break;
> +		}

[Severity: High]
Can a later redirect still widen the verdict of an earlier one here?

The guard at the top of the loop lets a second FLOW_ACTION_REDIRECT
through after a redirect. This case then ORs another DPORTn bit into
action[2] and does not set terminal.

The comment on the guard only explains why a drop gives the same result
whatever the redirect's control is. That does not hold for a second
redirect: with STOLEN only the first port gets the packet, and with PIPE
both ports do.

Take a filter like:

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

iproute2 sets the redirect control to TC_ACT_STOLEN by default. In
software at ingress, tcf_mirred_can_reinsert(TC_ACT_STOLEN) is true. So
tcf_mirred_to_dev() sends the skb itself to swp1 without cloning it and
returns TC_ACT_CONSUMED. tcf_action_exec() then stops:

net/sched/act_api.c:tcf_action_exec() {
    ...
		if (ret != TC_ACT_PIPE)
			break;
    ...
}

The second redirect never runs.

The driver still accepts this rule, even with skip_sw. It programs the
hardware with FWD_REDIR and DPORTS = {swp1, swp2}, so matched frames are
also sent to swp2.

Dropping skip_sw gives the same result. A hardware redirect never reaches
the CPU, so software has no chance to correct it.

The commit message says:

    Consecutive redirects merge into the destination port mask, since a flow
    action entry carries no mirred control verdict to tell whether software
    would have continued; an action asking for a second decision is rejected
    when skip_sw is set and left to the software path otherwise.

tcf_mirred_offload_act_setup() only fills in entry->id and entry->dev, so
the driver cannot tell which case it has. Merging the ports picks the
PIPE behaviour.

Would it be better to send a second FLOW_ACTION_REDIRECT through the same
fallback path as other actions after a redirect? It would then be
rejected under skip_sw and trapped to the CPU otherwise.

The later patch in this series, "net: dsa: motorcomm: Delete the ACL rule
of the port being unbound", only changes yt921x_acl_find() and
yt921x_acl_del(). The merge is still in place at the end of the series.

[ ... ]

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