[PATCH nf-next v5 0/3] flow offload teardown when layer 2 roaming

[PATCH nf-next v5 0/3] flow offload teardown when layer 2 roaming

[Julius Bairaktaris]

A direct xmit flow keeps the bridge port and destination address that the
fdb resolved when the flow was created. When the host moves to another
port of the bridge, for example a station roaming to another access
point, the bridge moves the fdb entry and the flow keeps forwarding to the
old port. Every packet the sender keeps transmitting refreshes the flow,
so it does not age out, and a hardware offloaded flow never sees a packet
on the software path where the stale port could be corrected.

The bridge notifies SWITCHDEV_FDB_DEL_TO_DEVICE for the old port before it
moves the entry. Patch 2 tears down the matching direct xmit flows on that
notification. Patch 1 stores the bridge vid in the tuple so the vid of the
deleted entry can be compared. Patch 3 adds a selftest.

Tested on an ipq807x router (OpenWrt, fw4 flowtable with the offload flag)
with a station roaming between the two radios of one bridge during a
download: without the patches the download stalls for as long as the
station stays on the other radio and resumes only after it roams back,
with them it continues through the roam, three runs each. A host moving
from a switch port with a hardware offloaded flow to a radio gets the flow
torn down and re-offloaded towards the radio.

Changes in v5:
- Rebase onto nf-next; the forward path resolution now lives in
  nf_flow_table_path.c and the tuple has no hw_ifidx.
- Store the bridge port in the tuple and match on it: the out interface
  is the lowest device of the path and differs from the port when the
  port is a vlan device.
- Add a helper for the per-tuple match.
- Drop the WARN_ON on allocation failure; the flow then ages out as
  before, the same way dsa_user_fdb_event() drops the hardware fdb
  update when its work item cannot be allocated.
- Skip local fdb entries, kick the gc once per flowtable.
- Hold the device with netdev_hold()/netdev_put() and a tracker.
- Add a selftest that roams a host between two bridge ports, plain and
  vlan, mid-transfer.

Changes in v4:
- Removed patch "don't follow fastpath when marked teardown".
- Use a work queue to process the event.

Changes in v3:
- static nf_flow_table_switchdev_nb.

Changes in v2:
- Unchanged, only tags RFC net-next to PATCH nf-next.

v4: https://lore.kernel.org/netfilter-devel/20250925182623.114045-1-ericwouds@gmail.com/

Eric Woudstra (2):
  netfilter: flow: Add bridge_vid member
  netfilter: flowtable: teardown direct xmit flows when the fdb entry
    moves

Julius Bairaktaris (1):
  selftests: netfilter: nft_flowtable.sh: roam a host between two bridge
    ports

 include/net/netfilter/nf_flow_table.h         |   4 +
 net/netfilter/nf_flow_table_core.c            | 109 ++++++++++++++
 net/netfilter/nf_flow_table_path.c            |   9 ++
 tools/testing/selftests/net/netfilter/config  |   1 +
 .../selftests/net/netfilter/nft_flowtable.sh  | 140 ++++++++++++++++++
 5 files changed, 263 insertions(+)

-- 
2.53.0

[PATCH nf-next v5 1/3] netfilter: flow: Add bridge_vid member

[Julius Bairaktaris]

From: Eric Woudstra <ericwouds@gmail.com>

Store the vid used on the bridge in the flow_offload_tuple, so it can be
used later to identify fdb entries that relate to the tuple.

The bridge_vid member is added to the structures nft_forward_info,
nf_flow_route and flow_offload_tuple. It can now be passed from
net_device_path->bridge.vlan_id to flow_offload_tuple->out.bridge_vid.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
Assisted-by: LLM
---
 include/net/netfilter/nf_flow_table.h | 2 ++
 net/netfilter/nf_flow_table_core.c    | 1 +
 net/netfilter/nf_flow_table_path.c    | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index f2e2771f188f..cadd5f82be8e 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -164,6 +164,7 @@ struct flow_offload_tuple {
 		};
 		struct {
 			u32		ifidx;
+			u16		bridge_vid;
 			u8		h_source[ETH_ALEN];
 			u8		h_dest[ETH_ALEN];
 		} out;
@@ -232,6 +233,7 @@ struct nf_flow_route {
 		struct {
 			u32			ifindex;
 			u32			hw_ifindex;
+			u16			bridge_vid;
 			u8			h_source[ETH_ALEN];
 			u8			h_dest[ETH_ALEN];
 			u8			needs_gso_segment:1;
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 03241d4bfd5e..b308fb627640 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -139,6 +139,7 @@ static int flow_offload_fill_route(struct flow_offload *flow,
 		memcpy(flow_tuple->out.h_source, route->tuple[dir].out.h_source,
 		       ETH_ALEN);
 		flow_tuple->out.ifidx = route->tuple[dir].out.ifindex;
+		flow_tuple->out.bridge_vid = route->tuple[dir].out.bridge_vid;
 		break;
 	case FLOW_OFFLOAD_XMIT_XFRM:
 	case FLOW_OFFLOAD_XMIT_NEIGH:
diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 1e55644f2edb..53629e9b2273 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -88,6 +88,7 @@ struct nft_forward_info {
 		__be16	proto;
 	} encap[NF_FLOW_TABLE_ENCAP_MAX];
 	u8 num_encaps;
+	u16 bridge_vid;
 	struct flow_offload_tunnel tun;
 	struct dst_entry *tun_dst;
 	u8 num_tuns;
@@ -180,6 +181,7 @@ static int nft_dev_path_info(struct net_device_path_stack *stack,
 			case DEV_PATH_BR_VLAN_KEEP:
 				break;
 			}
+			info->bridge_vid = path->bridge.vlan_id;
 			info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
 			break;
 		default:
@@ -257,6 +259,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 	if (info.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT) {
 		memcpy(route->tuple[dir].out.h_source, info.h_source, ETH_ALEN);
 		memcpy(route->tuple[dir].out.h_dest, info.h_dest, ETH_ALEN);
+		route->tuple[dir].out.bridge_vid = info.bridge_vid;
 		route->tuple[dir].xmit_type = info.xmit_type;
 	}
 	route->tuple[dir].out.needs_gso_segment = info.needs_gso_segment;
-- 
2.53.0

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

[Julius Bairaktaris]

From: Eric Woudstra <ericwouds@gmail.com>

A direct xmit flow stores the bridge port and the destination address that
the bridge fdb resolved when the flow was created. When the host moves to
another port of the same bridge, for example a station roaming to another
access point, the bridge updates its fdb entry and the flow keeps sending
to the old port. Packets from the other end keep hitting the flow and
refresh it, so it does not expire while the sender retransmits. A hardware
offloaded flow behaves the same, and none of its packets reaches the
software path.

Before the bridge moves an fdb entry to another port it notifies
SWITCHDEV_FDB_DEL_TO_DEVICE for the old port. Tear down every direct xmit
flow whose bridge port, vid and destination address match on that
notification. The bridge port is stored in the tuple for this: the out
interface is the lowest device of the path and differs from the port when
the port is a vlan device. The notifier runs in atomic context, so the
flows are walked from a workqueue.

The same notification is sent when an entry ages out or is flushed; a flow
torn down that way is offloaded again once the bridge relearns the host.
When the work item cannot be allocated the flow ages out as before, the
same as dsa_user_fdb_event() drops the hardware fdb update in that case.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
Co-developed-by: Julius Bairaktaris <julius@bairaktaris.de>
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
Assisted-by: LLM
---
 include/net/netfilter/nf_flow_table.h |   2 +
 net/netfilter/nf_flow_table_core.c    | 108 ++++++++++++++++++++++++++
 net/netfilter/nf_flow_table_path.c    |   6 ++
 3 files changed, 116 insertions(+)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index cadd5f82be8e..412044dd3a83 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -164,6 +164,7 @@ struct flow_offload_tuple {
 		};
 		struct {
 			u32		ifidx;
+			u32		bridge_ifidx;
 			u16		bridge_vid;
 			u8		h_source[ETH_ALEN];
 			u8		h_dest[ETH_ALEN];
@@ -233,6 +234,7 @@ struct nf_flow_route {
 		struct {
 			u32			ifindex;
 			u32			hw_ifindex;
+			u32			bridge_ifindex;
 			u16			bridge_vid;
 			u8			h_source[ETH_ALEN];
 			u8			h_dest[ETH_ALEN];
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index b308fb627640..e6e55a1cadaf 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -4,7 +4,9 @@
 #include <linux/module.h>
 #include <linux/netfilter.h>
 #include <linux/rhashtable.h>
+#include <linux/etherdevice.h>
 #include <linux/netdevice.h>
+#include <net/switchdev.h>
 #include <net/ip.h>
 #include <net/ip6_route.h>
 #include <net/netfilter/nf_tables.h>
@@ -139,6 +141,8 @@ static int flow_offload_fill_route(struct flow_offload *flow,
 		memcpy(flow_tuple->out.h_source, route->tuple[dir].out.h_source,
 		       ETH_ALEN);
 		flow_tuple->out.ifidx = route->tuple[dir].out.ifindex;
+		flow_tuple->out.bridge_ifidx =
+			route->tuple[dir].out.bridge_ifindex;
 		flow_tuple->out.bridge_vid = route->tuple[dir].out.bridge_vid;
 		break;
 	case FLOW_OFFLOAD_XMIT_XFRM:
@@ -761,6 +765,94 @@ void nf_flow_table_cleanup(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_cleanup);
 
+static struct workqueue_struct *nf_flow_fdb_del_wq;
+
+struct nf_flow_fdb_del_work {
+	struct work_struct	work;
+	struct net_device	*dev;
+	netdevice_tracker	dev_tracker;
+	u16			vid;
+	u8			addr[ETH_ALEN];
+	bool			found;
+};
+
+static bool nf_flow_tuple_fdb_match(const struct flow_offload_tuple *tuple,
+				    const struct nf_flow_fdb_del_work *fw)
+{
+	return tuple->xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
+	       tuple->out.bridge_ifidx == fw->dev->ifindex &&
+	       tuple->out.bridge_vid == fw->vid &&
+	       ether_addr_equal(tuple->out.h_dest, fw->addr);
+}
+
+static void nf_flow_table_do_fdb_del(struct nf_flowtable *flow_table,
+				     struct flow_offload *flow, void *data)
+{
+	struct nf_flow_fdb_del_work *fw = data;
+
+	if (nf_flow_tuple_fdb_match(&flow->tuplehash[0].tuple, fw) ||
+	    nf_flow_tuple_fdb_match(&flow->tuplehash[1].tuple, fw)) {
+		flow_offload_teardown(flow);
+		fw->found = true;
+	}
+}
+
+static void nf_flow_table_fdb_del_work(struct work_struct *work)
+{
+	struct nf_flow_fdb_del_work *fw;
+	struct nf_flowtable *flowtable;
+
+	fw = container_of(work, struct nf_flow_fdb_del_work, work);
+
+	mutex_lock(&flowtable_lock);
+	list_for_each_entry(flowtable, &flowtables, list) {
+		if (!net_eq(read_pnet(&flowtable->net), dev_net(fw->dev)))
+			continue;
+
+		fw->found = false;
+		nf_flow_table_iterate(flowtable, nf_flow_table_do_fdb_del, fw);
+		if (fw->found)
+			mod_delayed_work(system_power_efficient_wq,
+					 &flowtable->gc_work, 0);
+	}
+	mutex_unlock(&flowtable_lock);
+
+	netdev_put(fw->dev, &fw->dev_tracker);
+	kfree(fw);
+}
+
+/* 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;
+
+	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;
+}
+
+static struct notifier_block nf_flow_table_switchdev_nb = {
+	.notifier_call = nf_flow_table_switchdev_event,
+};
+
 void nf_flow_table_free(struct nf_flowtable *flow_table)
 {
 	mutex_lock(&flowtable_lock);
@@ -838,6 +930,16 @@ static int __init nf_flow_table_module_init(void)
 	if (ret)
 		goto out_offload;
 
+	nf_flow_fdb_del_wq = alloc_workqueue("nf_flow_fdb_del", WQ_UNBOUND, 0);
+	if (!nf_flow_fdb_del_wq) {
+		ret = -ENOMEM;
+		goto out_wq;
+	}
+
+	ret = register_switchdev_notifier(&nf_flow_table_switchdev_nb);
+	if (ret)
+		goto out_switchdev;
+
 	ret = nf_flow_register_bpf();
 	if (ret)
 		goto out_bpf;
@@ -845,6 +947,10 @@ static int __init nf_flow_table_module_init(void)
 	return 0;
 
 out_bpf:
+	unregister_switchdev_notifier(&nf_flow_table_switchdev_nb);
+out_switchdev:
+	destroy_workqueue(nf_flow_fdb_del_wq);
+out_wq:
 	nf_flow_table_offload_exit();
 out_offload:
 	unregister_pernet_subsys(&nf_flow_table_net_ops);
@@ -855,6 +961,8 @@ static int __init nf_flow_table_module_init(void)
 
 static void __exit nf_flow_table_module_exit(void)
 {
+	unregister_switchdev_notifier(&nf_flow_table_switchdev_nb);
+	destroy_workqueue(nf_flow_fdb_del_wq);
 	nf_flow_table_offload_exit();
 	unregister_pernet_subsys(&nf_flow_table_net_ops);
 	kmem_cache_destroy(flow_offload_cachep);
diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 53629e9b2273..231eeef50f73 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -88,6 +88,7 @@ struct nft_forward_info {
 		__be16	proto;
 	} encap[NF_FLOW_TABLE_ENCAP_MAX];
 	u8 num_encaps;
+	u32 bridge_ifidx;
 	u16 bridge_vid;
 	struct flow_offload_tunnel tun;
 	struct dst_entry *tun_dst;
@@ -181,6 +182,10 @@ static int nft_dev_path_info(struct net_device_path_stack *stack,
 			case DEV_PATH_BR_VLAN_KEEP:
 				break;
 			}
+			/* dev_fill_forward_path() adds the bridge port after
+			 * the bridge.
+			 */
+			info->bridge_ifidx = stack->path[i + 1].dev->ifindex;
 			info->bridge_vid = path->bridge.vlan_id;
 			info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
 			break;
@@ -259,6 +264,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 	if (info.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT) {
 		memcpy(route->tuple[dir].out.h_source, info.h_source, ETH_ALEN);
 		memcpy(route->tuple[dir].out.h_dest, info.h_dest, ETH_ALEN);
+		route->tuple[dir].out.bridge_ifindex = info.bridge_ifidx;
 		route->tuple[dir].out.bridge_vid = info.bridge_vid;
 		route->tuple[dir].xmit_type = info.xmit_type;
 	}
-- 
2.53.0

[PATCH nf-next v5 3/3] selftests: netfilter: nft_flowtable.sh: roam a host between two bridge ports

[Julius Bairaktaris]

Add a test where a host behind a bridge on the router moves to another
port of that bridge while its flow is offloaded, as a station moving
between two access points attached to ports of one bridge. The host keeps
its address and mac on a bridge of its own and roams by moving that
bridge's port, so only the fdb changes. A sender streams tcp to the host;
the test checks that the flow was offloaded before the roam, that the
host's received byte counter advances after it, and that the flow is
offloaded again. The test runs once with plain veth bridge ports and once
with vlan devices as bridge ports.

Without the teardown the reply direction keeps going to the old port and
the transfer stalls. CONFIG_NET_SWITCHDEV is needed for the bridge to
notify the fdb move.

Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
Assisted-by: LLM
---
 tools/testing/selftests/net/netfilter/config  |   1 +
 .../selftests/net/netfilter/nft_flowtable.sh  | 140 ++++++++++++++++++
 2 files changed, 141 insertions(+)

diff --git a/tools/testing/selftests/net/netfilter/config b/tools/testing/selftests/net/netfilter/config
index c3c121b6f300..d4e7249cb5b2 100644
--- a/tools/testing/selftests/net/netfilter/config
+++ b/tools/testing/selftests/net/netfilter/config
@@ -60,6 +60,7 @@ CONFIG_NET_NS=y
 CONFIG_NET_PKTGEN=m
 CONFIG_NET_SCH_HTB=m
 CONFIG_NET_SCH_NETEM=m
+CONFIG_NET_SWITCHDEV=y
 CONFIG_NET_VRF=y
 CONFIG_NF_CONNTRACK=m
 CONFIG_NF_CONNTRACK_EVENTS=y
diff --git a/tools/testing/selftests/net/netfilter/nft_flowtable.sh b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
index 449c518bd947..0d9f7d771fac 100755
--- a/tools/testing/selftests/net/netfilter/nft_flowtable.sh
+++ b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
@@ -851,6 +851,146 @@ test_ipip
 
 test_bridge
 
+# Roam a host between two ports of a bridge on the router while its flow is
+# offloaded, as a station moving between two access points attached to ports
+# of one bridge. The host keeps its address and mac on a bridge of its own
+# and roams by moving that bridge's port, so only the fdb changes. With a vid
+# the bridge ports are vlan devices, whose ifindex differs from the device
+# the flow transmits on. A flow that stays on the fast path leaves no more
+# than a few packets in the forward chain, the counter tells offloaded from
+# slow path.
+test_bridge_roam()
+{
+	local vid=$1
+	local p0=sr0 p1=sr1 r0=rr0 r1=rr1
+	local what="roaming on bridge${vid:+ with vlan ports}"
+	local dev old new slow cpid
+
+	setup_ns rns1 rnsr rns2
+
+	if ! ip -net "$rnsr" link add br0 type bridge 2>/dev/null; then
+		echo "SKIP: could not add bridge for $what"
+		[ "$ret" -eq 0 ] && ret=$ksft_skip
+		return
+	fi
+
+	ip link add sr0 netns "$rns1" type veth peer name rr0 netns "$rnsr"
+	ip link add sr1 netns "$rns1" type veth peer name rr1 netns "$rnsr"
+	ip link add rw0 netns "$rnsr" type veth peer name w0 netns "$rns2"
+	for dev in sr0 sr1; do
+		ip -net "$rns1" link set "$dev" up
+	done
+	for dev in rr0 rr1; do
+		ip -net "$rnsr" link set "$dev" up
+	done
+
+	if [ -n "$vid" ]; then
+		for dev in sr0 sr1; do
+			ip -net "$rns1" link add link "$dev" name "$dev.$vid" type vlan id "$vid"
+			ip -net "$rns1" link set "$dev.$vid" up
+		done
+		for dev in rr0 rr1; do
+			ip -net "$rnsr" link add link "$dev" name "$dev.$vid" type vlan id "$vid"
+			ip -net "$rnsr" link set "$dev.$vid" up
+		done
+		p0=sr0.$vid; p1=sr1.$vid; r0=rr0.$vid; r1=rr1.$vid
+	fi
+
+	ip -net "$rnsr" link set "$r0" master br0
+	ip -net "$rnsr" link set "$r1" master br0
+	ip -net "$rnsr" addr add 10.9.1.1/24 dev br0
+	ip -net "$rnsr" link set br0 up
+	ip -net "$rnsr" addr add 10.9.2.1/24 dev rw0
+	ip -net "$rnsr" link set rw0 up
+	ip netns exec "$rnsr" sysctl -q net.ipv4.ip_forward=1
+
+	ip -net "$rns1" link add stbr type bridge
+	ip -net "$rns1" link set stbr address 02:00:00:09:01:99
+	ip -net "$rns1" link set "$p0" master stbr
+	ip -net "$rns1" link set stbr up
+	ip -net "$rns1" addr add 10.9.1.99/24 dev stbr
+	ip -net "$rns1" route add default via 10.9.1.1
+
+	ip -net "$rns2" addr add 10.9.2.99/24 dev w0
+	ip -net "$rns2" link set w0 up
+	ip -net "$rns2" route add default via 10.9.2.1
+
+	if ! ip netns exec "$rnsr" nft -f - <<EOF
+table inet filter {
+  counter roam_repl { }
+  flowtable f1 { hook ingress priority 0; devices = { rr0, rr1, rw0 }; }
+  chain forward {
+    type filter hook forward priority 0; policy accept;
+    meta oif "rw0" tcp dport 12345 flow add @f1
+    meta iif "rw0" tcp sport 12345 counter name roam_repl flow add @f1
+  }
+}
+EOF
+	then
+		echo "SKIP: could not load ruleset for $what"
+		[ "$ret" -eq 0 ] && ret=$ksft_skip
+		return
+	fi
+
+	# the host discards what it receives, its socket byte counter is read
+	rx_bytes() { ip netns exec "$rns1" ss -tin "dst 10.9.2.99" 2>/dev/null |
+		grep -o 'bytes_received:[0-9]*' | head -1 | cut -d: -f2; }
+	rx_started() { local n; n=$(rx_bytes); [ "${n:-0}" -gt 0 ]; }
+	# forward chain packets of the reply direction since the last call
+	slow_pkts() { ip netns exec "$rnsr" nft reset counter inet filter roam_repl |
+		grep -o 'packets [0-9]*' | cut -d' ' -f2; }
+	roam_listener_ready() { ss -N "$rns2" -lnt -o "sport = :12345" | grep -q 12345; }
+
+	timeout "$SOCAT_TIMEOUT" ip netns exec "$rns2" socat -u \
+		OPEN:/dev/zero TCP-LISTEN:12345,reuseaddr &
+	busywait 1000 roam_listener_ready
+	timeout "$SOCAT_TIMEOUT" ip netns exec "$rns1" socat -u \
+		TCP:10.9.2.99:12345 OPEN:/dev/null &
+	cpid=$!
+
+	busywait "$BUSYWAIT_TIMEOUT" rx_started
+	sleep 2
+	slow_pkts > /dev/null
+	sleep 2
+	slow=$(slow_pkts)
+	if [ "${slow:-0}" -gt 100 ]; then
+		echo "FAIL: $what: $slow forward chain packets before the roam" 1>&2
+		ret=1
+		kill "$cpid" 2>/dev/null
+		return
+	fi
+
+	# roam: move the host bridge to the other port and send on it
+	ip -net "$rns1" link set "$p0" nomaster
+	ip -net "$rns1" link set "$p1" master stbr
+	ip netns exec "$rns1" ping -c 2 -W 1 -q 10.9.1.1 >/dev/null 2>&1
+
+	# the bytes in flight at the roam drain within the first seconds; a
+	# stale flow delivers nothing after that, a torn down one resumes and
+	# is offloaded again.
+	sleep 4
+	old=$(rx_bytes)
+	sleep 4
+	new=$(rx_bytes)
+	slow_pkts > /dev/null
+	sleep 2
+	slow=$(slow_pkts)
+	kill "$cpid" 2>/dev/null
+
+	if [ "${new:-0}" -le "${old:-0}" ]; then
+		echo "FAIL: $what: transfer stalled after the roam" 1>&2
+		ret=1
+	elif [ "${slow:-0}" -gt 100 ]; then
+		echo "FAIL: $what: $slow forward chain packets after the roam" 1>&2
+		ret=1
+	else
+		echo "PASS: flow offload for $what"
+	fi
+}
+
+test_bridge_roam ""
+test_bridge_roam 100
+
 KEY_SHA="0x"$(ps -af | sha1sum | cut -d " " -f 1)
 KEY_AES="0x"$(ps -af | md5sum | cut -d " " -f 1)
 SPI1=$RANDOM
-- 
2.53.0

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

[netdev-bot+sashiko]

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.

> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index b308fb627640..e6e55a1cadaf 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c

[ ... ]

> @@ -761,6 +765,94 @@ void nf_flow_table_cleanup(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(nf_flow_table_cleanup);
>  
> +static struct workqueue_struct *nf_flow_fdb_del_wq;
> +
> +struct nf_flow_fdb_del_work {
> +	struct work_struct	work;
> +	struct net_device	*dev;
> +	netdevice_tracker	dev_tracker;
> +	u16			vid;
> +	u8			addr[ETH_ALEN];
> +	bool			found;
> +};
> +
> +static bool nf_flow_tuple_fdb_match(const struct flow_offload_tuple *tuple,
> +				    const struct nf_flow_fdb_del_work *fw)
> +{
> +	return tuple->xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
> +	       tuple->out.bridge_ifidx == fw->dev->ifindex &&
> +	       tuple->out.bridge_vid == fw->vid &&
> +	       ether_addr_equal(tuple->out.h_dest, fw->addr);
> +}
> +
> +static void nf_flow_table_do_fdb_del(struct nf_flowtable *flow_table,
> +				     struct flow_offload *flow, void *data)
> +{
> +	struct nf_flow_fdb_del_work *fw = data;
> +
> +	if (nf_flow_tuple_fdb_match(&flow->tuplehash[0].tuple, fw) ||
> +	    nf_flow_tuple_fdb_match(&flow->tuplehash[1].tuple, fw)) {
> +		flow_offload_teardown(flow);
> +		fw->found = true;
> +	}
> +}
> +
> +static void nf_flow_table_fdb_del_work(struct work_struct *work)
> +{
> +	struct nf_flow_fdb_del_work *fw;
> +	struct nf_flowtable *flowtable;
> +
> +	fw = container_of(work, struct nf_flow_fdb_del_work, work);
> +
> +	mutex_lock(&flowtable_lock);
> +	list_for_each_entry(flowtable, &flowtables, list) {
> +		if (!net_eq(read_pnet(&flowtable->net), dev_net(fw->dev)))
> +			continue;
> +
> +		fw->found = false;
> +		nf_flow_table_iterate(flowtable, nf_flow_table_do_fdb_del, fw);
> +		if (fw->found)
> +			mod_delayed_work(system_power_efficient_wq,
> +					 &flowtable->gc_work, 0);
> +	}
> +	mutex_unlock(&flowtable_lock);
> +
> +	netdev_put(fw->dev, &fw->dev_tracker);
> +	kfree(fw);
> +}

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

> +
> +static struct notifier_block nf_flow_table_switchdev_nb = {
> +	.notifier_call = nf_flow_table_switchdev_event,
> +};
> +
>  void nf_flow_table_free(struct nf_flowtable *flow_table)
>  {
>  	mutex_lock(&flowtable_lock);
> @@ -838,6 +930,16 @@ static int __init nf_flow_table_module_init(void)
>  	if (ret)
>  		goto out_offload;
>  
> +	nf_flow_fdb_del_wq = alloc_workqueue("nf_flow_fdb_del", WQ_UNBOUND, 0);
> +	if (!nf_flow_fdb_del_wq) {
> +		ret = -ENOMEM;
> +		goto out_wq;
> +	}
> +
> +	ret = register_switchdev_notifier(&nf_flow_table_switchdev_nb);
> +	if (ret)
> +		goto out_switchdev;
> +

[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

Re: [PATCH nf-next v5 3/3] selftests: netfilter: nft_flowtable.sh: roam a host between two bridge ports

[netdev-bot+sashiko]

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

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

- [Medium] In test_bridge_roam() the two readiness barriers are decision
  points whose failure is ignored: `busywait 1000 roam_listener_ready`…
- [Medium] Both offload assertions in test_bridge_roam() are one-sided
  upper bounds on the reply-direction forward-chain counter (`[…
- [Low] The listener socat started in $rns2 (nft_flowtable.sh:944-945) is
  backgrounded but its PID is never captured, so no exit path of…

--

selftests: netfilter: nft_flowtable.sh: roam a host between two bridge ports

This adds test_bridge_roam() to nft_flowtable.sh, which moves a host
bridge between two ports of a router bridge while a tcp flow is
offloaded, and checks that the transfer resumes and stays on the fast
path.  It is run twice, once with plain veth bridge ports and once with
vlan devices, and it adds CONFIG_NET_SWITCHDEV to the netfilter config.

A few questions about the new test below.

> diff --git a/tools/testing/selftests/net/netfilter/nft_flowtable.sh b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> index 449c518bd947..0d9f7d771fac 100755
> --- a/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> +++ b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> @@ -851,6 +851,146 @@ test_ipip
>  
>  test_bridge
>  
> +# Roam a host between two ports of a bridge on the router while its flow is
> +# offloaded, as a station moving between two access points attached to ports
> +# of one bridge. The host keeps its address and mac on a bridge of its own
> +# and roams by moving that bridge's port, so only the fdb changes. With a vid
> +# the bridge ports are vlan devices, whose ifindex differs from the device
> +# the flow transmits on. A flow that stays on the fast path leaves no more
> +# than a few packets in the forward chain, the counter tells offloaded from
> +# slow path.
> +test_bridge_roam()
> +{

[ ... namespace, veth, vlan, bridge and ruleset setup snipped ... ]

> +	# the host discards what it receives, its socket byte counter is read
> +	rx_bytes() { ip netns exec "$rns1" ss -tin "dst 10.9.2.99" 2>/dev/null |
> +		grep -o 'bytes_received:[0-9]*' | head -1 | cut -d: -f2; }
> +	rx_started() { local n; n=$(rx_bytes); [ "${n:-0}" -gt 0 ]; }
> +	# forward chain packets of the reply direction since the last call
> +	slow_pkts() { ip netns exec "$rnsr" nft reset counter inet filter roam_repl |
> +		grep -o 'packets [0-9]*' | cut -d' ' -f2; }
> +	roam_listener_ready() { ss -N "$rns2" -lnt -o "sport = :12345" | grep -q 12345; }
> +
> +	timeout "$SOCAT_TIMEOUT" ip netns exec "$rns2" socat -u \
> +		OPEN:/dev/zero TCP-LISTEN:12345,reuseaddr &

[Severity: Low]
The listener socat in $rns2 is backgrounded but its pid is not saved, so
no exit path of test_bridge_roam() can stop it.  Only the client pid is
kept in $cpid, and neither child is ever waited for.  Elsewhere in this
same file the listener pid is captured and reaped:

	timeout "$SOCAT_TIMEOUT" ip netns exec "$nsb" socat ... &
	lpid=$!
	...
	wait $lpid

Would it be worth following that pattern here?

The leftover process does get reclaimed, since cleanup_ns() in
tools/testing/selftests/net/lib.sh does

	ip netns pids "${ns}" 2> /dev/null | xargs -r kill || true

and that runs both from setup_ns() when the second call test_bridge_roam 100
reuses the names, and from the EXIT trap via cleanup_all_ns().  Each socat
is also bounded by timeout "$SOCAT_TIMEOUT".  So this is hygiene rather
than a leak.

> +	busywait 1000 roam_listener_ready
> +	timeout "$SOCAT_TIMEOUT" ip netns exec "$rns1" socat -u \
> +		TCP:10.9.2.99:12345 OPEN:/dev/null &
> +	cpid=$!
> +
> +	busywait "$BUSYWAIT_TIMEOUT" rx_started

[Severity: Medium]
Can a setup failure here be reported as a flowtable regression?  The exit
status of both readiness barriers is discarded.

busywait() in tools/testing/selftests/net/lib.sh does return the status of
loopy_wait():

	busywait()
	{
		local timeout_ms=$1; shift

		loopy_wait : "$timeout_ms" "$@"
	}

If the listener is not bound within the hard-coded 1000 ms, the client
socat is started anyway and exits immediately on ECONNREFUSED with no
retry.  busywait "$BUSYWAIT_TIMEOUT" rx_started then also times out
unnoticed, and the test continues to measure a connection that never
existed.

With no traffic at all, slow_pkts() returns 0, so the pre-roam check
[ "${slow:-0}" -gt 100 ] is false and control falls through to the roam.
Afterwards old=$(rx_bytes) and new=$(rx_bytes) are both the empty string,
so [ "${new:-0}" -le "${old:-0}" ] evaluates 0 -le 0 and the script sets
ret=1 with "FAIL: ... transfer stalled after the roam".

Would it be better to check these two busywait calls and report an error
or a skip when the stream never comes up, rather than letting a broken
precondition turn into a fail attributed to the roam path?

> +	sleep 2
> +	slow_pkts > /dev/null
> +	sleep 2
> +	slow=$(slow_pkts)
> +	if [ "${slow:-0}" -gt 100 ]; then
> +		echo "FAIL: $what: $slow forward chain packets before the roam" 1>&2
> +		ret=1
> +		kill "$cpid" 2>/dev/null
> +		return
> +	fi
> +
> +	# roam: move the host bridge to the other port and send on it
> +	ip -net "$rns1" link set "$p0" nomaster
> +	ip -net "$rns1" link set "$p1" master stbr
> +	ip netns exec "$rns1" ping -c 2 -W 1 -q 10.9.1.1 >/dev/null 2>&1
> +
> +	# the bytes in flight at the roam drain within the first seconds; a
> +	# stale flow delivers nothing after that, a torn down one resumes and
> +	# is offloaded again.
> +	sleep 4
> +	old=$(rx_bytes)
> +	sleep 4
> +	new=$(rx_bytes)
> +	slow_pkts > /dev/null
> +	sleep 2
> +	slow=$(slow_pkts)
> +	kill "$cpid" 2>/dev/null
> +
> +	if [ "${new:-0}" -le "${old:-0}" ]; then
> +		echo "FAIL: $what: transfer stalled after the roam" 1>&2
> +		ret=1
> +	elif [ "${slow:-0}" -gt 100 ]; then
> +		echo "FAIL: $what: $slow forward chain packets after the roam" 1>&2
> +		ret=1
> +	else
> +		echo "PASS: flow offload for $what"
> +	fi

[Severity: Medium]
Both offload assertions in test_bridge_roam() are one-sided upper bounds
on the reply-direction counter, [ "${slow:-0}" -gt 100 ], with nothing
tying them to traffic actually having been observed.  A reading of 0
packets is accepted as evidence of offload.

slow_pkts() is a pipeline and the script does not set pipefail:

	slow_pkts() { ip netns exec "$rnsr" nft reset counter inet filter roam_repl |
		grep -o 'packets [0-9]*' | cut -d' ' -f2; }

so a failing nft reset counter, or a grep that matches nothing, yields an
empty string which ${slow:-0} silently turns into 0.

The progress check is similarly weak: [ "${new:-0}" -le "${old:-0}" ] is
satisfied by a single byte of advance over the eight seconds around the
roam.

The commit message says the test "checks that the flow was offloaded
before the roam".  Since that check passes when no flow exists and when
the counter read fails, could a lower bound be added, for example
requiring that the pre-roam byte counter advanced by a meaningful amount
and that the counter read succeeded, before concluding the flow was on the
fast path?

> +}
> +
> +test_bridge_roam ""
> +test_bridge_roam 100

[ ... ]

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