Re: [PATCH net-next] net: dsa: sja1105: flower: reject cross-chip redirect

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, May 29, 2026 at 04:35:44AM +0800, David Yang wrote:
> The driver silently accepts a destination port on a different switch
> chip, then programs its index (on another chip) into the local hardware,
> which redirects to the wrong port or possibly crashes if the port index
> is out of bound locally.
> 
> Add a check for it and adjust the extack message.
> 
> Signed-off-by: David Yang <mmyangfl@gmail.com>
> ---
>  drivers/net/dsa/sja1105/sja1105_flower.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
> index fba926f85b47..7547999a113f 100644
> --- a/drivers/net/dsa/sja1105/sja1105_flower.c
> +++ b/drivers/net/dsa/sja1105/sja1105_flower.c
> @@ -391,9 +391,9 @@ int sja1105_cls_flower_add(struct dsa_switch *ds, int port,
>  			struct dsa_port *to_dp;
>  
>  			to_dp = dsa_port_from_netdev(act->dev);
> -			if (IS_ERR(to_dp)) {
> +			if (IS_ERR(to_dp) || to_dp->ds != ds) {
>  				NL_SET_ERR_MSG_MOD(extack,
> -						   "Destination not a switch port");
> +						   "Destination not a local switch port");
>  				return -EOPNOTSUPP;
>  			}
>  
> -- 
> 2.53.0
> 

The problem is more nuanced, but I agree with the patch. If you want to
resend to make it more clear in the commit message, fine, if not, oh well.

Basically these days we can do better than rejecting the operation, see
https://lore.kernel.org/all/20241023135251.1752488-1-vladimir.oltean@nxp.com/

If cls->common.skip_sw is set, sure, but if not, just redirecting to
dsa_upstream_port(ds, port) should be fine (but not enough).

The problem is that the source user port itself may not belong to a
switch directly attached to the conduit, and in this case, redirecting
to the CPU implies programming the upstream switch(es) to also redirect
that flow to the CPU, for it to be properly processed exclusively in
software as opposed to flooded. And then there's a risk that the flow
may have been programmed on that switch already with another target, so
the operation might fail.

This appears to also be a problem with my patch set linked above, so
they should be treated together (not necessarily by you, not necessarily
now).

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>