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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5b332473-b552-489c-acd6-d0b67a1df098@gmail.com>
Date: Tue, 7 Oct 2025 11:49:13 +0200
From: Eric Woudstra <ericwouds@...il.com>
To: Florian Westphal <fw@...len.de>
Cc: Pablo Neira Ayuso <pablo@...filter.org>,
 Jozsef Kadlecsik <kadlec@...filter.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, Nikolay Aleksandrov <razor@...ckwall.org>,
 netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v4 nf-next 2/2] netfilter: nf_flow_table_core: teardown
 direct xmit when destination changed

On 10/2/25 9:55 AM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@...il.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.