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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aN4v1DB2S-AWTXAR@strlen.de>
Date: Thu, 2 Oct 2025 09:55:00 +0200
From: Florian Westphal <fw@...len.de>
To: Eric Woudstra <ericwouds@...il.com>
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

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?