Re: [net-next] net: dsa: offer a flowtable to the switch before the conduit

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] dsa_user_setup_tc() now selects the TC_SETUP_FT offload target
  (switch vs. conduit) independently on every call, by probing…
- [Low] The new comment at net/dsa/user.c:1743-1746 says '... only a
  conduit-side flow engine needs the block forwarded, and that forward…

--

net: dsa: offer a flowtable to the switch before the conduit

TC_SETUP_FT is now offered to the switch driver via
ds->ops->port_setup_tc() first, and only forwarded to the conduit netdev
via dsa_user_setup_ft_block() when the switch answers -EOPNOTSUPP. This
lets a switch that owns its own flow engine see a flowtable bound to one
of its user ports.

> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 041f9060c8ef0..c5adb557056f7 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -1734,11 +1734,22 @@ static int dsa_user_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  {
>  	struct dsa_port *dp = dsa_user_to_port(dev);
>  	struct dsa_switch *ds = dp->ds;
> +	int err;
>  
>  	switch (type) {
>  	case TC_SETUP_BLOCK:
>  		return dsa_user_setup_tc_block(dev, type_data);
>  	case TC_SETUP_FT:
> +		/* A switch that owns the flow tables answers for itself; only
> +		 * a conduit-side flow engine needs the block forwarded, and
> +		 * that forward is gone by the time the port is torn down.
> +		 */

[Severity: Low]
The comment says "that forward is gone by the time the port is torn
down", but the statement right below it runs that conduit forward on
every TC_SETUP_FT command, teardown unbind included. After -EOPNOTSUPP
from the switch, dsa_user_setup_ft_block() still resolves
dsa_port_to_conduit() and calls conduit->netdev_ops->ndo_setup_tc().
Nothing suppresses the forward during teardown.

The changelog's argument is about the other direction — a conduit-side
handler reaching back through conduit->dsa_ptr, which does not work
because dsa_tree_teardown() in net/dsa/dsa.c orders the teardown as:

	dsa_tree_teardown_conduit(dst);

	dsa_tree_teardown_ports(dst);

That ordering claim checks out, so this looks like just the comment
wording talking about a route the patch does not take. Could the comment
be reworded so it describes the fallback that is actually emitted here?

> +		if (ds->ops->port_setup_tc) {
> +			err = ds->ops->port_setup_tc(ds, dp->index, type,
> +						     type_data);
> +			if (err != -EOPNOTSUPP)
> +				return err;
> +		}
>  		return dsa_user_setup_ft_block(ds, dp->index, type_data);

[Severity: Medium]
TC_SETUP_FT carries a stateful FLOW_BLOCK_BIND / FLOW_BLOCK_UNBIND pair,
but this picks the offload target fresh on every call from the driver's
return code. Nothing records which side accepted the bind.

If a switch driver's TC_SETUP_FT verdict differs between the bind and the
matching unbind — for example its flow engine is unavailable at bind time
and available later, or offload is toggled at runtime — does the unbind
then land on the other target?

The target that did not see the bind has no matching block_cb, so its
flow_block_cb_lookup() fails and it returns -ENOENT. Since -ENOENT is not
-EOPNOTSUPP, dsa_user_setup_tc() propagates it (and the conduit's return
value is propagated unconditionally by dsa_user_setup_ft_block()).

On the netfilter side that error short-circuits the cleanup in
net/netfilter/nf_flow_table_offload.c:

nf_flow_table_offload_cmd() {
	...
	down_write(&flowtable->flow_block_lock);
	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_FT, bo);
	up_write(&flowtable->flow_block_lock);
	if (err < 0)
		return err;
	...
}

nf_flow_table_offload_setup() {
	...
	if (err < 0)
		return err;

	return nf_flow_table_block_setup(flowtable, &bo, cmd);
}

nf_flow_table_block_setup() is the only place an unbind does list_del()
plus flow_block_cb_free(), so does a misrouted unbind leak the
flow_block_cb allocated at bind time, leaving it linked on the owner
driver's private block list with cb_priv pointing at flowtable state that
is going away?

Worth noting no in-tree .port_setup_tc handles TC_SETUP_FT today — they
all return -EOPNOTSUPP unconditionally — so this cannot trigger in tree
right now, and the behaviour of the out-of-tree consumer is unknown.
Would it make sense to decide the target only for FLOW_BLOCK_BIND and
remember it on the port, so the unbind always goes back to whoever
accepted the bind?

>  	default:
>  		break;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901092546.369232-1-julius%40bairaktaris.de