[PATCH net] netfilter: nf_flow_table: defer pernet stats release to last flowtable teardown

[PATCH net] netfilter: nf_flow_table: defer pernet stats release to last flowtable teardown

[Qihang]

The pernet flowtable statistics (net->ft.stat) are released when the
netns exits, but the flowtables attached to a dying netns are torn down
asynchronously afterwards: the tc conntrack action frees its flowtable
from an RCU-delayed workqueue item (tcf_ct_flow_table_cleanup_work()),
and nf_tables releases flowtables from the transaction release work.

nf_flow_table_free() ends with a garbage-collection pass that queues
offload works; flow_offload_queue_work() increments the pernet counters
when scheduling them and the handlers decrement the counters again once
they complete.  The flushes in nf_flow_table_free() only drain those
workqueues when hardware offload is in use, so after free_percpu() the
stray workers still this_cpu_inc()/this_cpu_dec() into freed percpu
memory.

Account flowtables at init time and defer the release of the statistics
to the last nf_flow_table_free() of a dismantled netns, after draining
the offload workqueues.  A netns without flowtables keeps the
synchronous release.

Fixes: b038177636f8 ("netfilter: nf_flow_table: count pending offload workqueue tasks")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 include/net/netfilter/nf_flow_table.h |  1 +
 include/net/netns/flow_table.h        | 11 +++++++
 net/netfilter/nf_flow_table_core.c    | 44 ++++++++++++++++++++++++++-
 net/netfilter/nf_flow_table_offload.c |  7 +++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index 5b22d31cb1407..91ffa3072a0cc 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -376,6 +376,7 @@ void nf_flow_offload_stats(struct nf_flowtable *flowtable,
 
 void nf_flow_table_offload_flush(struct nf_flowtable *flowtable);
 void nf_flow_table_offload_flush_cleanup(struct nf_flowtable *flowtable);
+void nf_flow_table_offload_drain(void);
 
 int nf_flow_table_offload_setup(struct nf_flowtable *flowtable,
 				struct net_device *dev,
diff --git a/include/net/netns/flow_table.h b/include/net/netns/flow_table.h
index 1c5fc657e2675..6d2dc0d1f06c7 100644
--- a/include/net/netns/flow_table.h
+++ b/include/net/netns/flow_table.h
@@ -2,6 +2,9 @@
 #ifndef __NETNS_FLOW_TABLE_H
 #define __NETNS_FLOW_TABLE_H
 
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
 struct nf_flow_table_stat {
 	unsigned int count_wq_add;
 	unsigned int count_wq_del;
@@ -9,6 +12,14 @@ struct nf_flow_table_stat {
 };
 
 struct netns_ft {
+	/* Protects nr_ft and exiting. */
+	spinlock_t		lock;
+	/* Number of flowtables attached to this netns. */
+	int			nr_ft;
+	/* Set at netns exit; the statistics are then released by the
+	 * last flowtable teardown.
+	 */
+	bool			exiting;
 	struct nf_flow_table_stat __percpu *stat;
 };
 #endif
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 0d72dc8ae47a1..de0a9f0e4f7b9 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -715,6 +715,7 @@ EXPORT_SYMBOL_GPL(nf_flow_dnat_port);
 
 int nf_flow_table_init(struct nf_flowtable *flowtable)
 {
+	struct net *net;
 	int err;
 
 	INIT_DELAYED_WORK(&flowtable->gc_work, nf_flow_offload_work_gc);
@@ -733,6 +734,13 @@ int nf_flow_table_init(struct nf_flowtable *flowtable)
 	list_add(&flowtable->list, &flowtables);
 	mutex_unlock(&flowtable_lock);
 
+	net = read_pnet(&flowtable->net);
+	if (net) {
+		spin_lock(&net->ft.lock);
+		net->ft.nr_ft++;
+		spin_unlock(&net->ft.lock);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_init);
@@ -771,6 +779,8 @@ EXPORT_SYMBOL_GPL(nf_flow_table_cleanup);
 
 void nf_flow_table_free(struct nf_flowtable *flow_table)
 {
+	struct net *net;
+
 	mutex_lock(&flowtable_lock);
 	list_del(&flow_table->list);
 	mutex_unlock(&flowtable_lock);
@@ -781,19 +791,51 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
 	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
 	nf_flow_table_gc_run(flow_table);
 	nf_flow_table_offload_flush_cleanup(flow_table);
+
+	/* The gc pass above can still queue offload works whose handlers
+	 * decrement the pernet statistics.  Drain them all.
+	 */
+	nf_flow_table_offload_drain();
 	rhashtable_destroy(&flow_table->rhashtable);
+
+	net = read_pnet(&flow_table->net);
+	if (net) {
+		bool release;
+
+		spin_lock(&net->ft.lock);
+		release = net->ft.exiting && !--net->ft.nr_ft;
+		spin_unlock(&net->ft.lock);
+
+		/* Released by the last flowtable teardown, see
+		 * nf_flow_table_fini_net().
+		 */
+		if (release)
+			free_percpu(net->ft.stat);
+	}
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_free);
 
 static int nf_flow_table_init_net(struct net *net)
 {
+	spin_lock_init(&net->ft.lock);
 	net->ft.stat = alloc_percpu(struct nf_flow_table_stat);
 	return net->ft.stat ? 0 : -ENOMEM;
 }
 
 static void nf_flow_table_fini_net(struct net *net)
 {
-	free_percpu(net->ft.stat);
+	bool release;
+
+	spin_lock(&net->ft.lock);
+	net->ft.exiting = true;
+	release = !net->ft.nr_ft;
+	spin_unlock(&net->ft.lock);
+
+	/* Pending flowtables still update the statistics from workqueue
+	 * context; the last one releases them, see nf_flow_table_free().
+	 */
+	if (release)
+		free_percpu(net->ft.stat);
 }
 
 static int nf_flow_table_pernet_init(struct net *net)
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 4365859220e6a..6ac28802ef632 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -1180,6 +1180,13 @@ void nf_flow_table_offload_flush(struct nf_flowtable *flowtable)
 	}
 }
 
+void nf_flow_table_offload_drain(void)
+{
+	flush_workqueue(nf_flow_offload_add_wq);
+	flush_workqueue(nf_flow_offload_del_wq);
+	flush_workqueue(nf_flow_offload_stats_wq);
+}
+
 static int nf_flow_table_block_setup(struct nf_flowtable *flowtable,
 				     struct flow_block_offload *bo,
 				     enum flow_block_command cmd)
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net] netfilter: nf_flow_table: defer pernet stats release to last flowtable teardown

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] netfilter: nf_flow_table: defer pernet stats release to last flowtable teardown

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In nf_flow_table_free() (net/netfilter/nf_flow_table_core.c),
  the line `release = net->ft.exiting && !--net->ft.nr_ft;` only…
- [High] The deferred-release scheme keeps its state (ft.lock, ft.nr_ft,
  ft.exiting, ft.stat) inside struct net and expects the last…
- [High] nf_flow_table_fini_net() sets net->ft.exiting = true, but
  nf_flow_table_init_net() never resets exiting or nr_ft.
- [Medium] The commit message gets the failure mechanism wrong and does
  not mention a behaviour change.

--

netfilter: nf_flow_table: defer pernet stats release to last flowtable teardown

This commit counts flowtables per netns and adds an exiting flag, so
that the free_percpu() of net->ft.stat moves from netns exit to the
last nf_flow_table_free() of a dismantled netns. It also adds
nf_flow_table_offload_drain(), which flushes all three offload
workqueues on every nf_flow_table_free().

> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index 0d72dc8ae47a1..de0a9f0e4f7b9 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c

[ ... ]

> @@ -781,19 +791,51 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
>  	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>  	nf_flow_table_gc_run(flow_table);
>  	nf_flow_table_offload_flush_cleanup(flow_table);
> +
> +	/* The gc pass above can still queue offload works whose handlers
> +	 * decrement the pernet statistics.  Drain them all.
> +	 */
> +	nf_flow_table_offload_drain();

[Severity: Medium]
Does the commit message describe the failure correctly? It says:

  "The flushes in nf_flow_table_free() only drain those workqueues when
  hardware offload is in use, so after free_percpu() the stray workers
  still this_cpu_inc()/this_cpu_dec() into freed percpu memory."

As far as I can see, offload works are only queued for hardware offload
flowtables. flow_offload_add() calls nf_flow_offload_add() only when
nf_flowtable_hw_offload() is true. nf_flow_offload_gc_step() only queues
del/stats works for flows that have NF_FLOW_HW set:

    if (test_bit(NF_FLOW_HW, &flow->flags)) {

NFT_FLOWTABLE_HW_OFFLOAD also can't be toggled on update. So the flushes
that only run for hw offload don't seem to be the actual gap.

When fini_net runs before the flowtable is freed, the stats access
appears to come from the synchronous gc pass inside nf_flow_table_free()
itself, not only from stray workers:

  nf_flow_table_gc_run()
    flow_offload_queue_work()
      NF_FLOW_TABLE_STAT_INC_ATOMIC

Also, nf_flow_table_offload_drain() is called unconditionally here. That
includes software-only flowtables in live namespaces. It flushes
nf_flow_offload_add_wq, nf_flow_offload_del_wq and
nf_flow_offload_stats_wq. As a result, every flowtable deletion waits for
all pending hardware offload work in the system, driver callbacks
included.

The commit message presents the drain as part of the final release for a
dismantled netns. With Cc: stable, could the message also mention this
latency change?

>  	rhashtable_destroy(&flow_table->rhashtable);
> +
> +	net = read_pnet(&flow_table->net);
> +	if (net) {
> +		bool release;
> +
> +		spin_lock(&net->ft.lock);
> +		release = net->ft.exiting && !--net->ft.nr_ft;

[Severity: High]
Can this leak net->ft.stat for any netns that freed a flowtable before it
exited?

While the netns is alive, net->ft.exiting is false. The && then
short-circuits, so --net->ft.nr_ft never runs.

nf_flow_table_init() still increments nr_ft for every flowtable.
nf_tables_newflowtable() and tcf_ct_flow_table_get() both call
write_pnet() before nf_flow_table_init(), so net is always set there.

Take this sequence:

  create netns
  add an nft flowtable                      nr_ft = 1
  delete it
    nf_tables_flowtable_destroy()
      nf_flow_table_free()                  exiting == false, nr_ft stays 1
  destroy netns
    nf_flow_table_fini_net()
      release = !net->ft.nr_ft;             false

After this, nothing calls free_percpu(net->ft.stat).

Removing a tc ct action through tcf_ct_flow_table_cleanup_work() does the
same. So does the nf_tables_newflowtable() error path after a successful
init.

Should nr_ft be decremented unconditionally, with only the release
decision depending on exiting?

> +		spin_unlock(&net->ft.lock);
> +
> +		/* Released by the last flowtable teardown, see
> +		 * nf_flow_table_fini_net().
> +		 */
> +		if (release)
> +			free_percpu(net->ft.stat);
> +	}

[Severity: High]
Can this block run after struct net has already been freed?

The hand-off state (ft.lock, ft.nr_ft, ft.exiting, ft.stat) lives in
struct net. act_ct doesn't hold a netns reference on its flowtable.
tcf_ct_flow_table_get() does:

    write_pnet(&ct_ft->nf_ft.net, net);

with no get_net() or net_passive_inc(). Teardown then goes:

  tcf_ct_cleanup()
    call_rcu(tcf_ct_params_free_rcu)
      tcf_ct_flow_table_put()
        queue_rcu_work(act_ct_wq, &ct_ft->rwork)
          tcf_ct_flow_table_cleanup_work()
            nf_flow_table_free()

cleanup_net() runs the pernet exits, then rcu_barrier(), then
net_passive_dec(). A later cleanup_net() frees the net through
net_complete_free().

rcu_barrier() waits for the RCU callback that queues the work. It does
not wait for the work to run on act_ct_wq. Nothing in act_ct's pernet
exit flushes act_ct_wq, since ct_exit_net() only calls
tc_action_net_exit().

This is the case the patch targets: fini_net sees nr_ft > 0 and sets
exiting. If the deferred nf_flow_table_free() runs after struct net is
freed, it takes spin_lock(&net->ft.lock) and decrements nr_ft. It may
also call free_percpu() on a pointer read from freed or reused memory.

act_ct_wq is ordered. Each cleanup item now does
cancel_delayed_work_sync() plus three global flush_workqueue() calls, so
a backlog of ct_ft cleanups makes this window wider.

Would act_ct need to pin the netns, or should the stats live outside
struct net?

>  }
>  EXPORT_SYMBOL_GPL(nf_flow_table_free);
>  
>  static int nf_flow_table_init_net(struct net *net)
>  {
> +	spin_lock_init(&net->ft.lock);
>  	net->ft.stat = alloc_percpu(struct nf_flow_table_stat);
>  	return net->ft.stat ? 0 : -ENOMEM;
>  }
>  
>  static void nf_flow_table_fini_net(struct net *net)
>  {
> -	free_percpu(net->ft.stat);
> +	bool release;
> +
> +	spin_lock(&net->ft.lock);
> +	net->ft.exiting = true;

[Severity: High]
Is net->ft.exiting ever cleared again?

nf_flow_table_init_net() only calls spin_lock_init() and alloc_percpu().
struct netns_ft is embedded in struct net, so exiting and nr_ft survive a
module unload and reload. That holds for every existing netns, including
init_net.

Take this sequence:

  rmmod nf_flow_table
    nf_flow_table_module_exit()
      unregister_pernet_subsys(&nf_flow_table_net_ops)
        nf_flow_table_fini_net()
          exiting = true, stat freed if nr_ft == 0
  modprobe nf_flow_table
    nf_flow_table_init_net()
      stat re-allocated, exiting still true
  create and delete a flowtable
    nf_flow_table_free()
      release = net->ft.exiting && !--net->ft.nr_ft;   true
      free_percpu(net->ft.stat)                        netns still alive

>From then on, flow_offload_queue_work() and the work handlers update
freed percpu memory, and the procfs stats read it. At netns exit,
nf_flow_table_fini_net() sees nr_ft == 0 and frees the same pointer a
second time.

The out_proc error path in nf_flow_table_pernet_init() leaves the same
stale flag. So does the register_pernet_subsys() unwind when module init
fails, on namespaces that stay alive.

Should nf_flow_table_init_net() reset exiting and nr_ft?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105065509.94627-1-q.h.hack.winter%40gmail.com