[PATCH nf] netfilter: nf_tables: use RCU-safe list primitives for basechain hook list

[PATCH nf] netfilter: nf_tables: use RCU-safe list primitives for basechain hook list

[Weiming Shi]

NFT_MSG_GETCHAIN runs as an NFNL_CB_RCU callback, so chain dumps
traverse basechain->hook_list under rcu_read_lock() without holding
commit_mutex. Meanwhile, nft_delchain_hook() mutates that same live
hook_list with plain list_move() and list_splice(), and the commit/abort
paths splice hooks back with plain list_splice(). None of these are
RCU-safe list operations.

A concurrent GETCHAIN dump can observe partially updated list pointers,
follow them into stack-local or transaction-private list heads, and
crash when container_of() produces a bogus struct nft_hook pointer.

The PoC triggers this by racing GETCHAIN dumps against aborting DELCHAIN
hook updates, reachable from an unprivileged user namespace since all
capability checks use ns_capable() with CONFIG_NF_TABLES=y (default):

 Oops: general protection fault, probably for non-canonical address 0xdffffc0000000006: 0000 [#1] SMP KASAN NOPTI
 KASAN: null-ptr-deref in range [0x0000000000000030-0x0000000000000037]
 RIP: 0010:strlen (lib/string.c:420 (discriminator 1))
 Call Trace:
  <TASK>
  nf_tables_fill_chain_info (net/netfilter/nf_tables_api.c:1987 (discriminator 1) net/netfilter/nf_tables_api.c:1992 (discriminator 1) net/netfilter/nf_tables_api.c:2028 (discriminator 1) net/netfilter/nf_tables_api.c:2077 (discriminator 1))
  nf_tables_dump_chains (net/netfilter/nf_tables_api.c:2173 (discriminator 1))
  netlink_dump (net/netlink/af_netlink.c:2325 (discriminator 1))
  __netlink_dump_start (net/netlink/af_netlink.c:2442)
  nf_tables_getchain (net/netfilter/nf_tables_api.c:1314 net/netfilter/nf_tables_api.c:2212)
  nfnetlink_rcv_msg (net/netfilter/nfnetlink.c:290)
  netlink_rcv_skb (net/netlink/af_netlink.c:2550)
  nfnetlink_rcv (net/netfilter/nfnetlink.c:653)
  netlink_unicast (net/netlink/af_netlink.c:1319 net/netlink/af_netlink.c:1344)
  netlink_sendmsg (net/netlink/af_netlink.c:1894)
  __sys_sendto (net/socket.c:727 net/socket.c:742 net/socket.c:2206)
  __x64_sys_sendto (net/socket.c:2209)
  </TASK>

Replace list_move() in nft_delchain_hook() with list_del_rcu() plus an
intermediate pointer array, followed by synchronize_rcu() before the
deleted hooks' list pointers are reused to link them into the
transaction's private list. In the error paths, put hooks back with
list_add_tail_rcu() which is safe for concurrent RCU readers (they
either continue to the original successor or see the list head and
terminate the walk).

Add nft_hook_list_splice_rcu() helper that splices entries from a
private list into a live RCU-protected list using individual
list_add_tail_rcu() calls instead of plain list_splice(). Use it in
the commit and abort paths for NEWCHAIN updates and DELCHAIN rollback.

Fixes: 7d937b107108 ("netfilter: nf_tables: support for deleting devices in an existing netdev chain")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 net/netfilter/nf_tables_api.c | 64 ++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 8c42247a176c7..62fcfefba7b0f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -391,6 +391,22 @@ static void nft_netdev_unregister_hooks(struct net *net,
 	}
 }
 
+/* Splice hooks from a private list into a live (RCU-protected) hook list.
+ * Each entry is published individually via list_add_tail_rcu() so that
+ * concurrent RCU readers walking the destination list never observe torn
+ * list pointers.
+ */
+static void nft_hook_list_splice_rcu(struct list_head *from,
+				     struct list_head *to)
+{
+	struct nft_hook *hook, *next;
+
+	list_for_each_entry_safe(hook, next, from, list) {
+		list_del(&hook->list);
+		list_add_tail_rcu(&hook->list, to);
+	}
+}
+
 static int nf_tables_register_hook(struct net *net,
 				   const struct nft_table *table,
 				   struct nft_chain *chain)
@@ -3162,9 +3178,11 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
 	const struct nlattr * const *nla = ctx->nla;
 	struct nft_chain_hook chain_hook = {};
 	struct nft_hook *this, *hook;
+	struct nft_hook **del_hooks;
 	LIST_HEAD(chain_del_list);
 	struct nft_trans *trans;
-	int err;
+	int err, n = 0, i;
+	int max_hooks = 0;
 
 	if (ctx->table->flags & __NFT_TABLE_F_UPDATE)
 		return -EOPNOTSUPP;
@@ -3174,19 +3192,38 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
+	list_for_each_entry(this, &chain_hook.list, list)
+		max_hooks++;
+
+	del_hooks = kcalloc(max_hooks, sizeof(*del_hooks), GFP_KERNEL);
+	if (!del_hooks) {
+		nft_chain_release_hook(&chain_hook);
+		return -ENOMEM;
+	}
+
 	list_for_each_entry(this, &chain_hook.list, list) {
 		hook = nft_hook_list_find(&basechain->hook_list, this);
 		if (!hook) {
 			err = -ENOENT;
 			goto err_chain_del_hook;
 		}
-		list_move(&hook->list, &chain_del_list);
+		list_del_rcu(&hook->list);
+		del_hooks[n++] = hook;
 	}
 
+	/* Wait for any concurrent RCU readers (e.g. GETCHAIN dumps walking
+	 * basechain->hook_list) to finish before modifying the removed hooks'
+	 * list pointers to link them into the transaction's private list.
+	 */
+	synchronize_rcu();
+
+	for (i = 0; i < n; i++)
+		list_add_tail(&del_hooks[i]->list, &chain_del_list);
+
 	trans = nft_trans_alloc_chain(ctx, NFT_MSG_DELCHAIN);
 	if (!trans) {
 		err = -ENOMEM;
-		goto err_chain_del_hook;
+		goto err_chain_add_back;
 	}
 
 	nft_trans_basechain(trans) = basechain;
@@ -3194,13 +3231,24 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
 	INIT_LIST_HEAD(&nft_trans_chain_hooks(trans));
 	list_splice(&chain_del_list, &nft_trans_chain_hooks(trans));
 	nft_chain_release_hook(&chain_hook);
+	kfree(del_hooks);
 
 	nft_trans_commit_list_add_tail(ctx->net, trans);
 
 	return 0;
 
+err_chain_add_back:
+	for (i = 0; i < n; i++)
+		list_add_tail_rcu(&del_hooks[i]->list, &basechain->hook_list);
+	kfree(del_hooks);
+	nft_chain_release_hook(&chain_hook);
+
+	return err;
+
 err_chain_del_hook:
-	list_splice(&chain_del_list, &basechain->hook_list);
+	for (i = 0; i < n; i++)
+		list_add_tail_rcu(&del_hooks[i]->list, &basechain->hook_list);
+	kfree(del_hooks);
 	nft_chain_release_hook(&chain_hook);
 
 	return err;
@@ -10912,8 +10960,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 				nft_chain_commit_update(nft_trans_container_chain(trans));
 				nf_tables_chain_notify(&ctx, NFT_MSG_NEWCHAIN,
 						       &nft_trans_chain_hooks(trans));
-				list_splice(&nft_trans_chain_hooks(trans),
-					    &nft_trans_basechain(trans)->hook_list);
+				nft_hook_list_splice_rcu(&nft_trans_chain_hooks(trans),
+							&nft_trans_basechain(trans)->hook_list);
 				/* trans destroyed after rcu grace period */
 			} else {
 				nft_chain_commit_drop_policy(nft_trans_container_chain(trans));
@@ -11231,8 +11279,8 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 		case NFT_MSG_DELCHAIN:
 		case NFT_MSG_DESTROYCHAIN:
 			if (nft_trans_chain_update(trans)) {
-				list_splice(&nft_trans_chain_hooks(trans),
-					    &nft_trans_basechain(trans)->hook_list);
+				nft_hook_list_splice_rcu(&nft_trans_chain_hooks(trans),
+							&nft_trans_basechain(trans)->hook_list);
 			} else {
 				nft_use_inc_restore(&table->use);
 				nft_clear(trans->net, nft_trans_chain(trans));
-- 
2.43.0

Re: [PATCH nf] netfilter: nf_tables: use RCU-safe list primitives for basechain hook list

[Florian Westphal]

Weiming Shi <bestswngs@gmail.com> wrote:
> NFT_MSG_GETCHAIN runs as an NFNL_CB_RCU callback, so chain dumps
> traverse basechain->hook_list under rcu_read_lock() without holding
> commit_mutex. Meanwhile, nft_delchain_hook() mutates that same live
> hook_list with plain list_move() and list_splice(), and the commit/abort
> paths splice hooks back with plain list_splice(). None of these are
> RCU-safe list operations.
> 
> A concurrent GETCHAIN dump can observe partially updated list pointers,
> follow them into stack-local or transaction-private list heads, and
> crash when container_of() produces a bogus struct nft_hook pointer.

Right, but this is broken by design.

> Replace list_move() in nft_delchain_hook() with list_del_rcu() plus an
> intermediate pointer array, followed by synchronize_rcu() before the
> deleted hooks' list pointers are reused to link them into the
> transaction's private list. In the error paths, put hooks back with
> list_add_tail_rcu() which is safe for concurrent RCU readers (they
> either continue to the original successor or see the list head and
> terminate the walk).

I don't understand the existing code.

I don't even understand why
we have a difference between the 'update delete' and chain delete cases.

I think its wrong to unlink and then relink on abort.
What prevents nft_delchain_hook() from using the normal approach done
by nft_delchain()...?

This existing code appears to be way too complex.

Re: [PATCH nf] netfilter: nf_tables: use RCU-safe list primitives for basechain hook list

[Pablo Neira Ayuso]

On Fri, Apr 10, 2026 at 12:31:36PM +0200, Florian Westphal wrote:
> Weiming Shi <bestswngs@gmail.com> wrote:
[...]
> > Replace list_move() in nft_delchain_hook() with list_del_rcu() plus an
> > intermediate pointer array, followed by synchronize_rcu() before the
> > deleted hooks' list pointers are reused to link them into the
> > transaction's private list. In the error paths, put hooks back with
> > list_add_tail_rcu() which is safe for concurrent RCU readers (they
> > either continue to the original successor or see the list head and
> > terminate the walk).
> 
> I don't understand the existing code.

I am working on an alternative fix.