[nf-next PATCH 0/4] Address Sashiko review of NAT hook dump code

[nf-next PATCH 0/4] Address Sashiko review of NAT hook dump code

[Phil Sutter]

Patch 1 is mere preparation to add the missing case handling to
nfnl_hook_dump_nat(), the remaining three patches address the actual
concerns.

Link: https://sashiko.dev/#/patchset/20260702105003.13550-2-fw%40strlen.de

Phil Sutter (4):
  netfilter: nfnetlink_hook: Pass cb object to nfnl_hook_dump_nat()
  netfilter: nfnetlink_hook: Deref hook entry using READ_ONCE()
  netfilter: nfnetlink_hook: Handle multipart NAT hook dumps
  netfilter: nfnetlink_hook: Fix for concurrent NAT hooks dump and
    change

 net/netfilter/nfnetlink_hook.c | 43 ++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 12 deletions(-)

-- 
2.54.0

[nf-next PATCH 1/4] netfilter: nfnetlink_hook: Pass cb object to nfnl_hook_dump_nat()

[Phil Sutter]

Preparing for proper multipart dump handling, pass the object reference
directly instead of individual fields. Keep passing the 'family' field
though so caller does not have to extract that value from netlink header
again.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nfnetlink_hook.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
index 95005e9a6066..e47a2add4d5b 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -338,12 +338,12 @@ nfnl_hook_entries_head(u8 pf, unsigned int hook, struct net *net, const char *de
 }
 
 static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
-			      const struct nfnl_dump_hook_data *ctx,
-			      const struct nf_hook_ops *ops,
-			      int family, unsigned int seq)
+			      struct netlink_callback *cb,
+			      const struct nf_hook_ops *ops, int family)
 {
 	struct nf_nat_lookup_hook_priv *priv = ops->priv;
 	struct nf_hook_entries *e = rcu_dereference(priv->entries);
+	struct nfnl_dump_hook_data *ctx = cb->data;
 	struct nf_hook_ops **nat_ops;
 	int i, err;
 
@@ -354,7 +354,8 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
 
 	for (i = 0; i < e->num_hook_entries; i++) {
 		err = nfnl_hook_dump_one(nlskb, ctx, nat_ops[i],
-					 ops->priority, family, seq);
+					 ops->priority, family,
+					 cb->nlh->nlmsg_seq);
 		if (err)
 			return err;
 	}
@@ -390,8 +391,7 @@ static int nfnl_hook_dump(struct sk_buff *nlskb,
 
 	for (; i < e->num_hook_entries; i++) {
 		if (ops[i]->hook_ops_type == NF_HOOK_OP_NAT)
-			err = nfnl_hook_dump_nat(nlskb, ctx, ops[i], family,
-						 cb->nlh->nlmsg_seq);
+			err = nfnl_hook_dump_nat(nlskb, cb, ops[i], family);
 		else
 			err = nfnl_hook_dump_one(nlskb, ctx, ops[i],
 						 ops[i]->priority, family,
-- 
2.54.0

[nf-next PATCH 2/4] netfilter: nfnetlink_hook: Deref hook entry using READ_ONCE()

[Phil Sutter]

Writer (nf_remove_net_hook) assigns to the field value using
WRITE_ONCE(), appropriately call READ_ONCE() to make sure reader
(nfnl_hook_dump) sees either the old or new value, not both.

Fixes: b010e2a4a9ac ("netfilter: nfnetlink_hook: Dump nat type chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nfnetlink_hook.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
index e47a2add4d5b..efc674fc5adf 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -390,10 +390,12 @@ static int nfnl_hook_dump(struct sk_buff *nlskb,
 	ops = nf_hook_entries_get_hook_ops(e);
 
 	for (; i < e->num_hook_entries; i++) {
-		if (ops[i]->hook_ops_type == NF_HOOK_OP_NAT)
-			err = nfnl_hook_dump_nat(nlskb, cb, ops[i], family);
+		struct nf_hook_ops *cur = READ_ONCE(ops[i]);
+
+		if (cur->hook_ops_type == NF_HOOK_OP_NAT)
+			err = nfnl_hook_dump_nat(nlskb, cb, cur, family);
 		else
-			err = nfnl_hook_dump_one(nlskb, ctx, ops[i],
+			err = nfnl_hook_dump_one(nlskb, ctx, cur,
 						 ops[i]->priority, family,
 						 cb->nlh->nlmsg_seq);
 		if (err)
-- 
2.54.0

[nf-next PATCH 3/4] netfilter: nfnetlink_hook: Handle multipart NAT hook dumps

[Phil Sutter]

If the number of hooks exceeds available sk_buff space, the function is
called again for the remaining data. Make use of the second
netlink_callback::args field to store the current index between
invocations. Since this is an inner dump loop, it may run multiple times
(for different hook types) with the same context buffer, so manually
zero the stored value if complete array dump was possible.

Fixes: b010e2a4a9ac ("netfilter: nfnetlink_hook: Dump nat type chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nfnetlink_hook.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
index efc674fc5adf..0657fbb3e605 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -345,21 +345,26 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
 	struct nf_hook_entries *e = rcu_dereference(priv->entries);
 	struct nfnl_dump_hook_data *ctx = cb->data;
 	struct nf_hook_ops **nat_ops;
-	int i, err;
+	unsigned int i = cb->args[1];
+	int err = 0;
 
 	if (!e)
 		return 0;
 
 	nat_ops = nf_hook_entries_get_hook_ops(e);
 
-	for (i = 0; i < e->num_hook_entries; i++) {
+	for (; i < e->num_hook_entries; i++) {
 		err = nfnl_hook_dump_one(nlskb, ctx, nat_ops[i],
 					 ops->priority, family,
 					 cb->nlh->nlmsg_seq);
 		if (err)
-			return err;
+			break;
 	}
-	return 0;
+
+	if (!err)
+		i = 0;
+	cb->args[1] = i;
+	return err;
 }
 
 static int nfnl_hook_dump(struct sk_buff *nlskb,
-- 
2.54.0

[nf-next PATCH 4/4] netfilter: nfnetlink_hook: Fix for concurrent NAT hooks dump and change

[Phil Sutter]

If hooks are added or deleted, the nf_hook_entries buffer is reallocated
and the RCU-protected pointer updated. If a dump requires multiple parts
(exceeding sk_buff space) the second invocation may see a different
nf_hook_entries pointer than the first one. Detect this just like the
outer nfnl_hook_dump function does, by storing the pointer value in
context. If it changes, signal user space it must restart.

As with the stored loop counter, that stored pointer value must be
manually zeroed upon successful loop completion since this inner
function may run multiple times for different nf_hook_ops but with same
context.

Fixes: b010e2a4a9ac ("netfilter: nfnetlink_hook: Dump nat type chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nfnetlink_hook.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
index 0657fbb3e605..e01e59eddd64 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -55,6 +55,7 @@ static int nf_netlink_dump_start_rcu(struct sock *nlsk, struct sk_buff *skb,
 struct nfnl_dump_hook_data {
 	char devname[IFNAMSIZ];
 	unsigned long headv;
+	unsigned long natv;
 	u8 hook;
 };
 
@@ -351,6 +352,15 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
 	if (!e)
 		return 0;
 
+	if (!ctx->natv)
+		ctx->natv = (unsigned long)e;
+
+	if (i >= e->num_hook_entries ||
+	    ctx->natv != (unsigned long)e) {
+		cb->seq++;
+		return -EINTR;
+	}
+
 	nat_ops = nf_hook_entries_get_hook_ops(e);
 
 	for (; i < e->num_hook_entries; i++) {
@@ -361,8 +371,10 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
 			break;
 	}
 
-	if (!err)
+	if (!err) {
+		ctx->natv = 0
 		i = 0;
+	}
 	cb->args[1] = i;
 	return err;
 }
-- 
2.54.0

Re: [nf-next PATCH 4/4] netfilter: nfnetlink_hook: Fix for concurrent NAT hooks dump and change

[Phil Sutter]

On Wed, Jul 08, 2026 at 06:19:40PM +0200, Phil Sutter wrote:
[...]
> diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
> index 0657fbb3e605..e01e59eddd64 100644
> --- a/net/netfilter/nfnetlink_hook.c
> +++ b/net/netfilter/nfnetlink_hook.c
[...]
> @@ -361,8 +371,10 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
>  			break;
>  	}
>  
> -	if (!err)
> +	if (!err) {
> +		ctx->natv = 0

There's a typo here (missing semi-colon), kindly spotted by kernel test
robot. I'll send a v2 once it has cooled down a bit.

Sorry, Phil

Re: [nf-next PATCH 2/4] netfilter: nfnetlink_hook: Deref hook entry using READ_ONCE()

[Pablo Neira Ayuso]

Hi Phil,

On Wed, Jul 08, 2026 at 06:19:38PM +0200, Phil Sutter wrote:
> Writer (nf_remove_net_hook) assigns to the field value using
> WRITE_ONCE(), appropriately call READ_ONCE() to make sure reader
> (nfnl_hook_dump) sees either the old or new value, not both.

A bit broader question here:

Are we sure net/netfilter/core.c is safe to be walked over rcu in its
current state? Could the dummy_ops be exposed through nfnetlink_hook?

Maybe net/netfilter/core.c needs a revisited to use
rcu_assign_pointer() to assign the hook_ops to the blob, then
nfnetlink_hook uses rcu_dereference() instead of READ_ONCE.
Then the RCU semantics of the hooks would exposed in a better way?

That would made double use of RCU, one from the blob and then for the
hook_ops.

The hooks are now released using kfree_rcu(), at least in the recent
nf_nat core updates they are.

> Fixes: b010e2a4a9ac ("netfilter: nfnetlink_hook: Dump nat type chains")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/nfnetlink_hook.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
> index e47a2add4d5b..efc674fc5adf 100644
> --- a/net/netfilter/nfnetlink_hook.c
> +++ b/net/netfilter/nfnetlink_hook.c
> @@ -390,10 +390,12 @@ static int nfnl_hook_dump(struct sk_buff *nlskb,
>  	ops = nf_hook_entries_get_hook_ops(e);
>  
>  	for (; i < e->num_hook_entries; i++) {
> -		if (ops[i]->hook_ops_type == NF_HOOK_OP_NAT)
> -			err = nfnl_hook_dump_nat(nlskb, cb, ops[i], family);
> +		struct nf_hook_ops *cur = READ_ONCE(ops[i]);
> +
> +		if (cur->hook_ops_type == NF_HOOK_OP_NAT)
> +			err = nfnl_hook_dump_nat(nlskb, cb, cur, family);
>  		else
> -			err = nfnl_hook_dump_one(nlskb, ctx, ops[i],
> +			err = nfnl_hook_dump_one(nlskb, ctx, cur,
>  						 ops[i]->priority, family,
>  						 cb->nlh->nlmsg_seq);
>  		if (err)
> -- 
> 2.54.0
> 

Re: [nf-next PATCH 2/4] netfilter: nfnetlink_hook: Deref hook entry using READ_ONCE()

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Are we sure net/netfilter/core.c is safe to be walked over rcu in its
> current state? Could the dummy_ops be exposed through nfnetlink_hook?

What do you mean with 'safe'?
The walk is safe from memory safety point of view.

dummy_ops *can* be exposed.

Otherwise, hook unregister can fail when low on memory:
ATM, in case we unregister hook and then fail to alloc the replacement
blob (that is same as live one minus the removed hook) we leave the
dummy stub in so old hook function is no longer executed and leave the
outdated/stale blob in place.

One alternative to dummy-ops usage is to keep a spare blob around so we
can avoid the new memory allocation when a hook goes away.

Then, on delete:

1. use the spare (which is large enough) instead
   and prepare the new blob (without removed fn).
2. swap the spare with live version.
3. attempt to allocate a new spare.
   if that fails, force a synchronize_rcu() and make
   the 'old' live the new spare.
   Else, use the new spare and avoid the,
   synchronize_rcu(), old-live is handed off to call_rcu.

Hook-add would always have to keep the size of the spare
up to date, so it is always large enough to hold the
current amount of live hooks.

Its a bit more work, but it avoids the need for dummy_ops.
LLM should be able to generate the transformation patches.

> Maybe net/netfilter/core.c needs a revisited to use
> rcu_assign_pointer() to assign the hook_ops to the blob, then
> nfnetlink_hook uses rcu_dereference() instead of READ_ONCE.

Why?  What is the concern?

> Then the RCU semantics of the hooks would exposed in a better way?
>
> That would made double use of RCU, one from the blob and then for the
> hook_ops.

Its technically not needed, I think, the hook_ops are not supposed
to be munged while the blob is active.

Adding extra rcu_dereference adds additional barriers for each hook/elem
in the blob...

> The hooks are now released using kfree_rcu(), at least in the recent
> nf_nat core updates they are.

Yes.

Re: [nf-next PATCH 2/4] netfilter: nfnetlink_hook: Deref hook entry using READ_ONCE()

[Pablo Neira Ayuso]

On Thu, Jul 09, 2026 at 02:36:12PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Are we sure net/netfilter/core.c is safe to be walked over rcu in its
> > current state? Could the dummy_ops be exposed through nfnetlink_hook?
> 
> What do you mean with 'safe'?
> The walk is safe from memory safety point of view.
> 
> dummy_ops *can* be exposed.
> 
> Otherwise, hook unregister can fail when low on memory:
> ATM, in case we unregister hook and then fail to alloc the replacement
> blob (that is same as live one minus the removed hook) we leave the
> dummy stub in so old hook function is no longer executed and leave the
> outdated/stale blob in place.
> 
> One alternative to dummy-ops usage is to keep a spare blob around so we
> can avoid the new memory allocation when a hook goes away.
> 
> Then, on delete:
> 
> 1. use the spare (which is large enough) instead
>    and prepare the new blob (without removed fn).
> 2. swap the spare with live version.
> 3. attempt to allocate a new spare.
>    if that fails, force a synchronize_rcu() and make
>    the 'old' live the new spare.
>    Else, use the new spare and avoid the,
>    synchronize_rcu(), old-live is handed off to call_rcu.
> 
> Hook-add would always have to keep the size of the spare
> up to date, so it is always large enough to hold the
> current amount of live hooks.
> 
> Its a bit more work, but it avoids the need for dummy_ops.
> LLM should be able to generate the transformation patches.

Maybe a more simple way is to skip dummy_ops in the netlink dump so it
is not exposed to userspace, that's all.