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

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.

[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().

[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?

[ ... ]

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