Re: [PATCH net-next v2] 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: 1 · Medium: 1 · Low: 0

- [High] dsa_user_setup_ft_block() (net/dsa/user.c:1721) records which
  side accepted a flowtable bind in a single per-port boolean,…
- [Medium] In dsa_user_setup_ft_block() (net/dsa/user.c:1734) the
  ownership record is computed as `dp->ft_on_switch = err !=…

--

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

TC_SETUP_FT on a DSA user port is now offered to the switch driver via
ds->ops->port_setup_tc first, and only forwarded to the conduit netdev when
the switch answers -EOPNOTSUPP.  The side that accepted the bind is recorded
in a new per-port boolean, dp->ft_on_switch, so that the matching unbind is
sent to the same side.

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 7507d632e7c6d..dbcb02de06fce 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -297,6 +297,12 @@ struct dsa_port {
>  
>  	u8			setup:1;
>  
> +	/* Flowtable blocks on this user port go to the switch, not the
> +	 * conduit; decided at bind time. Written without rtnl, so not one of
> +	 * the bit fields above.
> +	 */
> +	bool			ft_on_switch;
> +

[Severity: High]
Can a single boolean per dsa_port track something the flow block API keys
per block?

nf_flow_table_block_offload_init() sets bo->block = &flowtable->flow_block,
so each flowtable is a distinct binding.  And more than one flowtable can
bind the same DSA user netdev, because the duplicate device check in
nft_register_flowtable_net_hooks() only looks at flowtables of the same nft
table:

net/netfilter/nf_tables_api.c:nft_register_flowtable_net_hooks() {
	list_for_each_entry(ft, &table->flowtables, list) {
		if (!nft_is_active_next(net, ft))
			continue;
		if (nft_hook_list_find(&ft->hook_list, hook)) {
			err = -EEXIST;
			...
}

Two flowtables in two different nft tables therefore produce two independent
FLOW_BLOCK_BINDs on the same port, and both write the one flag.  Compare
with dsa_user_setup_tc_block(), which keys the block on f->block via
flow_block_cb_lookup().

>  	struct device_node	*dn;
>  	unsigned int		ageing_time;
>  
> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 041f9060c8ef0..f4cd48df2519d 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -1718,15 +1718,30 @@ static int dsa_user_setup_tc_block(struct net_device *dev,
>  	}
>  }
>  
> -static int dsa_user_setup_ft_block(struct dsa_switch *ds, int port,
> -				   void *type_data)
> +static int dsa_user_setup_ft_block(struct dsa_port *dp,
> +				   struct flow_block_offload *bo)
>  {
> -	struct net_device *conduit = dsa_port_to_conduit(dsa_to_port(ds, port));
> +	struct net_device *conduit = dsa_port_to_conduit(dp);
> +	struct dsa_switch *ds = dp->ds;
> +	int err;
> +
> +	/* The unbind goes to the side that took the bind. */
> +	if (bo->command == FLOW_BLOCK_BIND) {
> +		err = -EOPNOTSUPP;
> +		if (ds->ops->port_setup_tc)
> +			err = ds->ops->port_setup_tc(ds, dp->index, TC_SETUP_FT,
> +						     bo);
> +		dp->ft_on_switch = err != -EOPNOTSUPP;
> +		if (dp->ft_on_switch)
> +			return err;
> +	} else if (dp->ft_on_switch) {
> +		return ds->ops->port_setup_tc(ds, dp->index, TC_SETUP_FT, bo);
> +	}

[Severity: High]
Following on from the per-block question above: since each
FLOW_BLOCK_BIND overwrites dp->ft_on_switch, and the FLOW_BLOCK_UNBIND
routing decision reads only that flag and never looks at bo->block, can an
unbind reach the side that never took the bind?

	BIND(FT_A)   switch returns 0            -> ft_on_switch = true
	BIND(FT_B)   switch has no room for a
	             second block, -EOPNOTSUPP   -> ft_on_switch = false,
	                                            conduit takes FT_B
	UNBIND(FT_A) reads false                 -> goes to the conduit

The conduit provider looks its block up by bo->block, so it simply fails:

drivers/net/ethernet/mediatek/mtk_ppe_offload.c:mtk_eth_setup_tc_block() {
	...
	block_cb = flow_block_cb_lookup(f->block, cb, dev);
	if (!block_cb)
		return -ENOENT;
	...
}

nft_unregister_flowtable_ops() discards that error, and
nf_flow_table_offload_setup() returns before reaching the freeing loop in
nf_flow_table_block_setup():

net/netfilter/nf_flow_table_offload.c:nf_flow_table_block_setup() {
	...
	case FLOW_BLOCK_UNBIND:
		list_for_each_entry_safe(block_cb, next, &bo->cb_list, list) {
			list_del(&block_cb->list);
			flow_block_cb_free(block_cb);
	...
}

nf_flow_table_free() has no leftover-cb cleanup either.  Does the switch's
flow_block_cb then stay linked on the driver's block list with ->block
pointing into the freed nf_flowtable, with the hardware offload entries
never torn down, so that a later flow_block_cb_lookup() can match a stale
cb if a new flowtable is allocated at the same address?  The mirrored
ordering (switch answers -EOPNOTSUPP for FT_A, accepts FT_B) sends a
conduit-owned unbind to the switch instead.

Would keying the ownership on bo->block, the way dsa_user_setup_tc_block()
does, avoid this?

[Severity: Medium]
Separately, should ft_on_switch be set for a bind that failed?

	dp->ft_on_switch = err != -EOPNOTSUPP;
	if (dp->ft_on_switch)
		return err;

Any hard error from ds->ops->port_setup_tc() (-ENOMEM, -EBUSY, -ENOSPC)
also satisfies err != -EOPNOTSUPP, so the port is latched as "the switch
owns the flowtable block" although nothing was bound anywhere -- the switch
failed, and the conduit was never offered the bind because of the early
return.  Nothing clears the flag afterwards; it is only recomputed on the
next FLOW_BLOCK_BIND, never on unbind.

An unbind for a different block that the conduit genuinely holds on the
same port then takes the else-if branch and is delivered to the switch:

	BIND(FT_A)   switch -EOPNOTSUPP     -> conduit takes FT_A
	BIND(FT_B)   switch -ENOMEM         -> ft_on_switch = true, err
	                                       returned, nothing bound
	UNBIND(FT_A) reads true             -> goes to the switch

The conduit's flow_block_cb for FT_A is then never removed and keeps a
->block pointer into the freed flowtable.  Should the flag only be set when
the switch actually accepted the bind, that is err == 0?

>  
>  	if (!conduit->netdev_ops->ndo_setup_tc)
>  		return -EOPNOTSUPP;
>  
> -	return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, type_data);
> +	return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, bo);
>  }
>  

[ ... ]

For both of the above: no in-tree .port_setup_tc handles TC_SETUP_FT today
(hellcreek, ksz, yt921x, mt753x, felix/vsc9959 and sja1105 all return
-EOPNOTSUPP), so the switch branch is not reachable in mainline yet, but
this patch is what enables such drivers.

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