Re: [PATCH nf-next v5 2/3] netfilter: flowtable: teardown direct xmit flows when the fdb entry moves

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

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

- [High] The new fdb-move teardown is wired exclusively to the switchdev
  atomic notifier chain, but the sole emitter of…
- [High] nf_flow_table_switchdev_event() unconditionally does one
  kzalloc_obj(GFP_ATOMIC) + netdev_hold() + queue_work() for *every*…
- [Medium] The teardown assumes SWITCHDEV_FDB_DEL_TO_DEVICE always
  precedes an fdb move, but br_switchdev_fdb_notify()…
- [Medium] The invalidator listens only for SWITCHDEV_FDB_DEL_TO_DEVICE,
  but two bridge fdb paths move an existing entry to a different port…
- [Medium] The teardown of matching direct-xmit flows is deferred to a
  workqueue, but the bridge emits SWITCHDEV_FDB_DEL_TO_DEVICE *before*…
- [Low] The commit fixes a user-visible forwarding failure (a direct-xmit
  flow keeps sending to the stale bridge port after a host roams, and is…

--

netfilter: flowtable: teardown direct xmit flows when the fdb entry moves

The patch stores the bridge port ifindex in the direct xmit flow tuple and
registers a switchdev notifier that queues a work item on
SWITCHDEV_FDB_DEL_TO_DEVICE. The work item walks all flowtables in the
namespace and tears down flows whose bridge port, vid and destination
address match the deleted fdb entry.

[Severity: Low]
There is no Fixes: tag and no statement about the intended tree.

The commit message describes a user-visible forwarding failure with no
workaround, since the peer's retransmissions keep refreshing the stale
flow. The stale bridge path came in with FLOW_OFFLOAD_XMIT_DIRECT in
commit 7a27f6ab4135 ("netfilter: flowtable: use dev_fill_forward_path() to
obtain egress device"), whose message already mentioned that moving a peer
to another bridge port leaves the connection on the old path until timeout,
and suggested snooping fdb updates.

Would a 'Fixes: 7a27f6ab4135 ("netfilter: flowtable: use
dev_fill_forward_path() to obtain egress device")' tag be appropriate here,
or is this intended as nf-next material because it adds new infrastructure
(a dedicated workqueue plus a global switchdev notifier)? As submitted it
is hard to tell which tree is targeted.

[ ... ]

[Severity: High]
How much work can a bridged segment generate through this path? Every
fdb deletion produces one work item, and each work item takes the global
flowtable_lock in nf_flow_table_fdb_del_work() across a full
nf_flow_table_iterate() walk of every flowtable in the namespace, plus an
immediate extra gc walk via mod_delayed_work(..., 0) on a match.

Bridge fdb deletions are data-plane driven. Aging notifies for every
learned entry:

net/bridge/br_fdb.c:br_fdb_cleanup() {
	...
		if (!hlist_unhashed(&f->fdb_node))
			fdb_delete(br, f, true);
	...
}

and a source-MAC port move notifies too:

net/bridge/br_fdb.c:br_fdb_update() {
	...
			br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
			WRITE_ONCE(fdb->dst, source);
	...
}

Both reach nf_flow_table_switchdev_event() through fdb_notify() ->
br_switchdev_fdb_notify() -> call_switchdev_notifiers().

So a MAC flood or MAC flap from a host on the bridged segment turns cheap
frames into (fdb deletions) x (flow entries) of walking under one global
mutex. Since flowtable_lock is a single static mutex shared by all
namespaces, does this also stall flowtable creation and teardown in other
netns? Each pending item also pins a netdev reference until the walk
finishes, which delays unregistration.

Would it help to filter before allocating, the way
dsa_user_switchdev_event() only handles devices it owns, and to coalesce
or bound the number of in-flight items?

+
+/* The bridge notifies the old port before it moves an fdb entry. Flows
+ * sending directly to that port are torn down. Skipping the teardown on
+ * allocation failure leaves the flow to age out, the same as a missed fdb
+ * update does for a switchdev driver.
+ */
+static int nf_flow_table_switchdev_event(struct notifier_block *nb,
+					 unsigned long event, void *ptr)
+{
+	struct switchdev_notifier_fdb_info *fdb_info = ptr;
+	struct nf_flow_fdb_del_work *fw;
+
+	if (event != SWITCHDEV_FDB_DEL_TO_DEVICE || fdb_info->is_local)
+		return NOTIFY_DONE;

[Severity: Medium]
Is SWITCHDEV_FDB_DEL_TO_DEVICE always emitted when an fdb entry moves?
br_switchdev_fdb_notify() drops it for two entry classes:

net/bridge/br_switchdev.c:br_switchdev_fdb_notify() {
	...
	if (test_bit(BR_FDB_LOCKED, &fdb->flags))
		return;
	...
	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
		return;
	...
}

br_fdb_update() still calls br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH)
and then commits WRITE_ONCE(fdb->dst, source) for those entries, so the
move happens but the request is swallowed inside the callee. That covers
locked-port entries and entries added with "bridge fdb add ... master
dynamic".

Flow creation does not exclude those entries either:

net/bridge/br_device.c:br_fill_forward_path() {
	...
	f = br_fdb_find_rcu(br, ctx->daddr, path->bridge.vlan_id);
	if (!f)
		return -1;

	dst = READ_ONCE(f->dst);
	...
}

so a direct xmit flow can be created against a locked or user-added
dynamic entry and then never torn down. Is a fallback trigger needed for
those cases?

[Severity: Medium]
Are the add-only fdb move paths covered? fdb_add_entry() changes the port
of an existing entry and reports it as a new entry:

net/bridge/br_fdb.c:fdb_add_entry() {
	...
		if (READ_ONCE(fdb->dst) != source) {
			WRITE_ONCE(fdb->dst, source);
			modified = true;
		}
	...
	fdb_notify(br, fdb, RTM_NEWNEIGH, true);
}

br_fdb_external_learn_add() does the same when an externally learned entry
changes port, overwriting fdb->dst and then calling fdb_notify(br, fdb,
RTM_NEWNEIGH, swdev_notify).

For a static entry that arrives as SWITCHDEV_FDB_ADD_TO_DEVICE, which the
filter above ignores, so a flow created while the entry pointed at the old
port keeps its stale bridge_ifidx and h_dest. Should the handler also act
on the add notification when the port changed, for example via "bridge fdb
replace ... master static" or a switchdev/VXLAN driver relearning the
entry?

+
+	fw = kzalloc_obj(*fw, GFP_ATOMIC);
+	if (!fw)
+		return NOTIFY_DONE;
+
+	INIT_WORK(&fw->work, nf_flow_table_fdb_del_work);
+	fw->dev = fdb_info->info.dev;
+	netdev_hold(fw->dev, &fw->dev_tracker, GFP_ATOMIC);
+	fw->vid = fdb_info->vid;
+	ether_addr_copy(fw->addr, fdb_info->addr);
+	queue_work(nf_flow_fdb_del_wq, &fw->work);
+
+	return NOTIFY_DONE;
+}

[Severity: Medium]
Can a flow created concurrently with the move escape the teardown walk?
The notification is sent before the new port is committed:

net/bridge/br_fdb.c:br_fdb_update() {
	...
			br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
			WRITE_ONCE(fdb->dst, source);
	...
}

so this is an "about to move" hint. Meanwhile a flow being set up on
another CPU resolves the port from the old dst:

net/netfilter/nf_flow_table_path.c:nft_dev_path_info() {
	...
		info->bridge_ifidx = stack->path[i + 1].dev->ifindex;
	...
}

and publishes the flow in flow_offload_add() with rhashtable_insert_fast(),
which does not take flowtable_lock. If the work item's walk completes
between the notification and that insertion, the stale flow is never seen.

br_fdb_update() only notifies when source != fdb->dst, so no further
SWITCHDEV_FDB_DEL_TO_DEVICE is generated for that host, and a later aging
DEL carries the new port, which cannot match tuple->out.bridge_ifidx.
Does the flow then persist for as long as return traffic refreshes it?

The reverse interleaving looks possible too: the work item carries no
event generation, so a delayed DEL can tear down a flow that was just
created against the new port after the address was relearned there.

[Severity: High]
What happens in a CONFIG_NET_SWITCHDEV=n build? The only emitter of the
bridge fdb notification is compiled out there:

net/bridge/Makefile:
	bridge-$(CONFIG_NET_SWITCHDEV) += br_switchdev.o

and net/bridge/br_private.h supplies an empty stub instead:

static inline void
br_switchdev_fdb_notify(struct net_bridge *br,
			const struct net_bridge_fdb_entry *fdb, int type)
{
}

On this side register_switchdev_notifier() is also a stub returning 0 when
CONFIG_NET_SWITCHDEV=n, so nf_flow_table_module_init() sees success,
creates the "nf_flow_fdb_del" unbound workqueue and installs a notifier
block that can never be called.

The roaming station scenario in the commit message needs no switchdev
hardware, so a plain software bridge router with CONFIG_BRIDGE=y,
CONFIG_NF_FLOW_TABLE=m and CONFIG_NET_SWITCHDEV=n still keeps sending to
the old port. config NF_FLOW_TABLE in net/netfilter/Kconfig depends only on
NETFILTER_INGRESS, NF_CONNTRACK and NF_TABLES, so nothing expresses the new
dependency. Should this get a depends on NET_SWITCHDEV, an IS_ENABLED()
guard around the workqueue and notifier, or a bridge level notification
instead?

 	ret = nf_flow_register_bpf();
 	if (ret)
 		goto out_bpf;

[ ... ]

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