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

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

[Eric Woudstra]

This patch-set can be reviewed separately from my submissions concerning
the bridge-fastpath.

In case of a bridge in the forward-fastpath or bridge-fastpath the fdb is
used to create the tuple. In case of roaming at layer 2 level, for example
802.11r, the destination device is changed in the fdb. The destination
device of a direct transmitting tuple is no longer valid and traffic is
send to the wrong destination. Also the hardware offloaded fastpath is not
valid anymore.

This flowentry needs to be torn down asap.

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.

Eric Woudstra (2):
  netfilter: flow: Add bridge_vid member
  netfilter: nf_flow_table_core: teardown direct xmit when destination
    changed

 include/net/netfilter/nf_flow_table.h |  2 +
 net/netfilter/nf_flow_table_core.c    | 89 +++++++++++++++++++++++++++
 net/netfilter/nft_flow_offload.c      |  3 +
 3 files changed, 94 insertions(+)

-- 
2.50.0

[PATCH v4 nf-next 1/2] netfilter: flow: Add bridge_vid member

[Eric Woudstra]

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>
---
 include/net/netfilter/nf_flow_table.h | 2 ++
 net/netfilter/nf_flow_table_core.c    | 1 +
 net/netfilter/nft_flow_offload.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 c003cd194fa2..bac3b0e9e3a1 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -146,6 +146,7 @@ struct flow_offload_tuple {
 		struct {
 			u32		ifidx;
 			u32		hw_ifidx;
+			u16		bridge_vid;
 			u8		h_source[ETH_ALEN];
 			u8		h_dest[ETH_ALEN];
 		} out;
@@ -212,6 +213,7 @@ struct nf_flow_route {
 		struct {
 			u32			ifindex;
 			u32			hw_ifindex;
+			u16			bridge_vid;
 			u8			h_source[ETH_ALEN];
 			u8			h_dest[ETH_ALEN];
 		} out;
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 9441ac3d8c1a..992958db4a19 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -128,6 +128,7 @@ static int flow_offload_fill_route(struct flow_offload *flow,
 		       ETH_ALEN);
 		flow_tuple->out.ifidx = route->tuple[dir].out.ifindex;
 		flow_tuple->out.hw_ifidx = route->tuple[dir].out.hw_ifindex;
+		flow_tuple->out.bridge_vid = route->tuple[dir].out.bridge_vid;
 		dst_release(dst);
 		break;
 	case FLOW_OFFLOAD_XMIT_XFRM:
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index 2148c4cde9e4..788bffbfac78 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -85,6 +85,7 @@ struct nft_forward_info {
 		__u16	id;
 		__be16	proto;
 	} encap[NF_FLOW_TABLE_ENCAP_MAX];
+	u16 bridge_vid;
 	u8 num_encaps;
 	u8 ingress_vlans;
 	u8 h_source[ETH_ALEN];
@@ -159,6 +160,7 @@ static void nft_dev_path_info(const 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:
@@ -223,6 +225,7 @@ static void nft_dev_forward_path(struct nf_flow_route *route,
 		memcpy(route->tuple[dir].out.h_dest, info.h_dest, ETH_ALEN);
 		route->tuple[dir].out.ifindex = info.outdev->ifindex;
 		route->tuple[dir].out.hw_ifindex = info.hw_outdev->ifindex;
+		route->tuple[dir].out.bridge_vid = info.bridge_vid;
 		route->tuple[dir].xmit_type = info.xmit_type;
 	}
 }
-- 
2.50.0

[PATCH v4 nf-next 2/2] netfilter: nf_flow_table_core: teardown direct xmit when destination changed

[Eric Woudstra]

In case of a bridge in the forward-fastpath or bridge-fastpath the fdb is
used to create the tuple. In case of roaming at layer 2 level, for example
802.11r, the destination device is changed in the fdb. The destination
device of a direct transmitting tuple is no longer valid and traffic is
send to the wrong destination. Also the hardware offloaded fastpath is not
valid anymore.

In case of roaming, a switchdev notification is send to delete the old fdb
entry. Upon receiving this notification, mark all direct transmitting flows
with the same ifindex, vid and hardware address as the fdb entry to be
teared down. The hardware offloaded fastpath is still in effect, so
minimize the delay of the work queue by setting the delay to zero.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 net/netfilter/nf_flow_table_core.c | 88 ++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 992958db4a19..e906f93f4abb 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -13,6 +13,7 @@
 #include <net/netfilter/nf_conntrack_core.h>
 #include <net/netfilter/nf_conntrack_l4proto.h>
 #include <net/netfilter/nf_conntrack_tuple.h>
+#include <net/switchdev.h>
 
 static DEFINE_MUTEX(flowtable_lock);
 static LIST_HEAD(flowtables);
@@ -745,6 +746,86 @@ void nf_flow_table_cleanup(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_cleanup);
 
+struct flow_cleanup_data {
+	int ifindex;
+	u16 vid;
+	char addr[ETH_ALEN];
+	bool found;
+};
+
+struct flow_switchdev_event_work {
+	struct work_struct work;
+	struct flow_cleanup_data cud;
+};
+
+static void nf_flow_table_do_cleanup_addr(struct nf_flowtable *flow_table,
+					  struct flow_offload *flow, void *data)
+{
+	struct flow_cleanup_data *cud = data;
+
+	if ((flow->tuplehash[0].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
+	     flow->tuplehash[0].tuple.out.ifidx == cud->ifindex &&
+	     flow->tuplehash[0].tuple.out.bridge_vid == cud->vid &&
+	     ether_addr_equal(flow->tuplehash[0].tuple.out.h_dest, cud->addr)) ||
+	    (flow->tuplehash[1].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
+	     flow->tuplehash[1].tuple.out.ifidx == cud->ifindex &&
+	     flow->tuplehash[1].tuple.out.bridge_vid == cud->vid &&
+	     ether_addr_equal(flow->tuplehash[1].tuple.out.h_dest, cud->addr))) {
+		flow_offload_teardown(flow);
+		cud->found = true;
+	}
+}
+
+static void nf_flow_table_switchdev_event_work(struct work_struct *work)
+{
+	struct flow_switchdev_event_work *switchdev_work =
+		container_of(work, struct flow_switchdev_event_work, work);
+	struct nf_flowtable *flowtable;
+
+	mutex_lock(&flowtable_lock);
+
+	list_for_each_entry(flowtable, &flowtables, list) {
+		switchdev_work->cud.found = false;
+		nf_flow_table_iterate(flowtable, nf_flow_table_do_cleanup_addr,
+				      &switchdev_work->cud);
+		if (switchdev_work->cud.found)
+			mod_delayed_work(system_power_efficient_wq,
+					 &flowtable->gc_work, 0);
+	}
+
+	mutex_unlock(&flowtable_lock);
+
+	kfree(switchdev_work);
+}
+
+static int nf_flow_table_switchdev_event(struct notifier_block *unused,
+					 unsigned long event, void *ptr)
+{
+	struct flow_switchdev_event_work *switchdev_work;
+	struct switchdev_notifier_fdb_info *fdb_info;
+
+	if (event != SWITCHDEV_FDB_DEL_TO_DEVICE)
+		return NOTIFY_DONE;
+
+	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
+	if (WARN_ON(!switchdev_work))
+		return NOTIFY_BAD;
+
+	INIT_WORK(&switchdev_work->work, nf_flow_table_switchdev_event_work);
+	fdb_info = ptr;
+	switchdev_work->cud.ifindex = fdb_info->info.dev->ifindex;
+	switchdev_work->cud.vid = fdb_info->vid;
+	ether_addr_copy(switchdev_work->cud.addr, fdb_info->addr);
+
+	queue_work(system_long_wq, &switchdev_work->work);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block nf_flow_table_switchdev_nb __read_mostly = {
+	.notifier_call = nf_flow_table_switchdev_event,
+};
+
 void nf_flow_table_free(struct nf_flowtable *flow_table)
 {
 	mutex_lock(&flowtable_lock);
@@ -818,6 +899,10 @@ static int __init nf_flow_table_module_init(void)
 	if (ret)
 		goto out_offload;
 
+	ret = register_switchdev_notifier(&nf_flow_table_switchdev_nb);
+	if (ret < 0)
+		goto out_sw_noti;
+
 	ret = nf_flow_register_bpf();
 	if (ret)
 		goto out_bpf;
@@ -825,6 +910,8 @@ static int __init nf_flow_table_module_init(void)
 	return 0;
 
 out_bpf:
+	unregister_switchdev_notifier(&nf_flow_table_switchdev_nb);
+out_sw_noti:
 	nf_flow_table_offload_exit();
 out_offload:
 	unregister_pernet_subsys(&nf_flow_table_net_ops);
@@ -833,6 +920,7 @@ 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);
 	nf_flow_table_offload_exit();
 	unregister_pernet_subsys(&nf_flow_table_net_ops);
 }
-- 
2.50.0

Re: [PATCH v4 nf-next 0/2] flow offload teardown when layer 2 roaming

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:
> This patch-set can be reviewed separately from my submissions concerning
> the bridge-fastpath.
> 
> In case of a bridge in the forward-fastpath or bridge-fastpath the fdb is
> used to create the tuple. In case of roaming at layer 2 level, for example
> 802.11r, the destination device is changed in the fdb.
               ~~~~~~~~~~~~~~~~~~

destination device == output port to use for xmit?

> The destination
> device of a direct transmitting tuple is no longer valid and traffic is
> send to the wrong destination. Also the hardware offloaded fastpath is not
> valid anymore.

Can you outline/summarize the existing behaviour for sw bridge, without
flowtable offload being in the mix here?

What is the existing behaviour without flowtable but bridge hw offload in place?
What mechanism corrects the output port in these cases?

> This flowentry needs to be torn down asap.

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

Full walk of flowtable is expensive, how many events
are expected to be generated?

Having a few thousands of fdb updates trigger one flowtable
walk each seems like a non-starter?

Re: [PATCH v4 nf-next 2/2] netfilter: nf_flow_table_core: teardown direct xmit when destination changed

[Florian Westphal]

Eric Woudstra <ericwouds@gmail.com> wrote:
> +static void nf_flow_table_do_cleanup_addr(struct nf_flowtable *flow_table,
> +					  struct flow_offload *flow, void *data)
> +{
> +	struct flow_cleanup_data *cud = data;
> +
> +	if ((flow->tuplehash[0].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
> +	     flow->tuplehash[0].tuple.out.ifidx == cud->ifindex &&
> +	     flow->tuplehash[0].tuple.out.bridge_vid == cud->vid &&
> +	     ether_addr_equal(flow->tuplehash[0].tuple.out.h_dest, cud->addr)) ||
> +	    (flow->tuplehash[1].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
> +	     flow->tuplehash[1].tuple.out.ifidx == cud->ifindex &&
> +	     flow->tuplehash[1].tuple.out.bridge_vid == cud->vid &&
> +	     ether_addr_equal(flow->tuplehash[1].tuple.out.h_dest, cud->addr))) {

I think it would be better to have a helper for this, so
it boils down to:
if (__nf_flow_table_do_cleanup_addr(flow->tuplehash[0]) ||
    __nf_flow_table_do_cleanup_addr(flow->tuplehash[1]))

(thats assuming we can go forward with the full walk.)

> +static int nf_flow_table_switchdev_event(struct notifier_block *unused,
> +					 unsigned long event, void *ptr)
> +{
> +	struct flow_switchdev_event_work *switchdev_work;
> +	struct switchdev_notifier_fdb_info *fdb_info;
> +
> +	if (event != SWITCHDEV_FDB_DEL_TO_DEVICE)
> +		return NOTIFY_DONE;
> +
> +	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
> +	if (WARN_ON(!switchdev_work))
> +		return NOTIFY_BAD;

No WARN_ON here.  GFP_ATOMIC can fail, which then gives a splat.
But there is nothing that could be done about it for either reporter
or developer.

So, how much of a problem is this?
If its fine to ignore the notification, then remove the WARN_ON.
If its not ok, then you have to explore alternatives that do not depend
on successful allocation.

Can the invalided output port be detected from packet path similar to
how stale dsts get handled?

Re: [PATCH v4 nf-next 0/2] flow offload teardown when layer 2 roaming

[Eric Woudstra]

On 10/2/25 9:47 AM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
>> This patch-set can be reviewed separately from my submissions concerning
>> the bridge-fastpath.
>>
>> In case of a bridge in the forward-fastpath or bridge-fastpath the fdb is
>> used to create the tuple. In case of roaming at layer 2 level, for example
>> 802.11r, the destination device is changed in the fdb.
>                ~~~~~~~~~~~~~~~~~~
> 
> destination device == output port to use for xmit?
> 

Indeed. It is the bridge-port that is being changed for the same
combination of vid and address. In the tuple it is the output port for xmit.

>> The destination
>> device of a direct transmitting tuple is no longer valid and traffic is
>> send to the wrong destination. Also the hardware offloaded fastpath is not
>> valid anymore.
> 
> Can you outline/summarize the existing behaviour for sw bridge, without
> flowtable offload being in the mix here?
> 
> What is the existing behaviour without flowtable but bridge hw offload in place?
> What mechanism corrects the output port in these cases?
> 

What is comes down to is br_fdb_update(), when an existing fdb entry is
found for the vid/address combination. When it appears on a different
bridge port then stored in the fdb entry, the fdb entry is modified.

Also br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH) is called so that
drivers can remove bridge hw offload. This is what I also want to listen
for, as it is the only message that holds to old bridge-port.

Listening in particular when it is called from br_fdb_update(), but it
can be debated if we should respond to all of these calls, or only when
called from br_fdb_update(). If we want to narrow it down, may need to
add an "updating" flag to:

struct switchdev_notifier_fdb_info {
	struct switchdev_notifier_info info; /* must be first */
	const unsigned char *addr;
	u16 vid;
	u8 added_by_user:1,
	   is_local:1,
	   locked:1,
	   offloaded:1;
+	   updating:1;
};

Or something similar.

>> This flowentry needs to be torn down asap.
> 
>> Changes in v4:
>> - Removed patch "don't follow fastpath when marked teardown".
>> - Use a work queue to process the event.
> 
> Full walk of flowtable is expensive, how many events
> are expected to be generated?
> 
> Having a few thousands of fdb updates trigger one flowtable
> walk each seems like a non-starter?

Indeed, this would be an argument to narrow it down. Fully walking
through the flowtable, only when an fdb entry's bridge-port is being
updated.

Re: [PATCH v4 nf-next 2/2] netfilter: nf_flow_table_core: teardown direct xmit when destination changed

[Eric Woudstra]

On 10/2/25 9:55 AM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@gmail.com> wrote:
>> +static void nf_flow_table_do_cleanup_addr(struct nf_flowtable *flow_table,
>> +					  struct flow_offload *flow, void *data)
>> +{
>> +	struct flow_cleanup_data *cud = data;
>> +
>> +	if ((flow->tuplehash[0].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
>> +	     flow->tuplehash[0].tuple.out.ifidx == cud->ifindex &&
>> +	     flow->tuplehash[0].tuple.out.bridge_vid == cud->vid &&
>> +	     ether_addr_equal(flow->tuplehash[0].tuple.out.h_dest, cud->addr)) ||
>> +	    (flow->tuplehash[1].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
>> +	     flow->tuplehash[1].tuple.out.ifidx == cud->ifindex &&
>> +	     flow->tuplehash[1].tuple.out.bridge_vid == cud->vid &&
>> +	     ether_addr_equal(flow->tuplehash[1].tuple.out.h_dest, cud->addr))) {
> 
> I think it would be better to have a helper for this, so
> it boils down to:
> if (__nf_flow_table_do_cleanup_addr(flow->tuplehash[0]) ||
>     __nf_flow_table_do_cleanup_addr(flow->tuplehash[1]))
> 
> (thats assuming we can go forward with the full walk.)
> 
>> +static int nf_flow_table_switchdev_event(struct notifier_block *unused,
>> +					 unsigned long event, void *ptr)
>> +{
>> +	struct flow_switchdev_event_work *switchdev_work;
>> +	struct switchdev_notifier_fdb_info *fdb_info;
>> +
>> +	if (event != SWITCHDEV_FDB_DEL_TO_DEVICE)
>> +		return NOTIFY_DONE;
>> +
>> +	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
>> +	if (WARN_ON(!switchdev_work))
>> +		return NOTIFY_BAD;
> 
> No WARN_ON here.  GFP_ATOMIC can fail, which then gives a splat.
> But there is nothing that could be done about it for either reporter
> or developer.
> 
> So, how much of a problem is this?
> If its fine to ignore the notification, then remove the WARN_ON.
> If its not ok, then you have to explore alternatives that do not depend
> on successful allocation.
> 
> Can the invalided output port be detected from packet path similar to
> how stale dsts get handled?

The flow needs to be torn down, when a wifi-client moves to another
bridge-port. Both old and new bridge-ports themselves are unchanged.
And in case of the flow being hardware offloaded, the flow also needs to
be torn down.