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

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

[Phil Sutter]

Changes since v2:
- Drop patch 1 again. Fixing nfnl_hook_dump's broken NLM_F_DUMP_INTR
  flag setting in corner-cases is out of scope of this series and should
  be solved in a more elegant way in nfnetlink itself.
Changes since v1:
- New patch 1
- Fixed three aspects of patch 3
- Fixed two issues of patch 5

Patch 1 is mere preparation to add the missing case handling to
nfnl_hook_dump_nat().

Patch 2 fixes for missing READ_ONCE() calls when addressing hook ops
array elements.

Patch 3 adds missing multipart dump support to nfnl_hook_dump_nat.

Patch 4 adds code to detect and mitigate concurrent hook updates while
amidst a multipart dump to nfnl_hook_dump_nat.

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: Address hook ops 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 | 48 ++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 14 deletions(-)

-- 
2.54.0

[nf-next PATCH v3 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 v3 2/4] netfilter: nfnetlink_hook: Address hook ops 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>
---
Changes since v1:
- Cover for previously missed ops[i]->priority parameter
- Also cover for nat_ops field access, adjust patch subject
- Declare temporary variable as const
---
 net/netfilter/nfnetlink_hook.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
index e47a2add4d5b..644070c8b456 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -353,7 +353,8 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
 	nat_ops = nf_hook_entries_get_hook_ops(e);
 
 	for (i = 0; i < e->num_hook_entries; i++) {
-		err = nfnl_hook_dump_one(nlskb, ctx, nat_ops[i],
+		err = nfnl_hook_dump_one(nlskb, ctx,
+					 READ_ONCE(nat_ops[i]),
 					 ops->priority, family,
 					 cb->nlh->nlmsg_seq);
 		if (err)
@@ -390,11 +391,13 @@ 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);
+		const 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],
-						 ops[i]->priority, family,
+			err = nfnl_hook_dump_one(nlskb, ctx, cur,
+						 cur->priority, family,
 						 cb->nlh->nlmsg_seq);
 		if (err)
 			break;
-- 
2.54.0

[nf-next PATCH v3 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 644070c8b456..be2f99b678be 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -345,22 +345,27 @@ 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,
 					 READ_ONCE(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 v3 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>
---
Changes since v1:
- Fix a stupid typo introduced last minute when resolving checkpatch.pl
  complaints
- If 'e' becomes zero, it also indicates an interrupted dump
---
 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 be2f99b678be..ad135419d08f 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;
 };
 
@@ -348,6 +349,15 @@ static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
 	unsigned int i = cb->args[1];
 	int err = 0;
 
+	if (!ctx->natv)
+		ctx->natv = (unsigned long)e;
+
+	if ((e && i >= e->num_hook_entries) ||
+	    ctx->natv != (unsigned long)e) {
+		cb->seq++;
+		return -EINTR;
+	}
+
 	if (!e)
 		return 0;
 
@@ -362,8 +372,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 v3 0/4] Address Sashiko review of NAT hook dump code

[Pablo Neira Ayuso]

Hi Phil,

I think we have to tackle this from a different angle the
nfnl_hook_dump_nat() dump issues, see:

static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
                              struct netlink_callback *cb,
                              const struct nf_hook_ops *ops, int family)
...

        if ((e && i >= e->num_hook_entries) ||
            ctx->natv != (unsigned long)e) {
                cb->seq++;
                return -EINTR;
        }

Because as sashiko reports, memory address could be recycled to defeat
this which is unlikely, but correct.

Instead of making heuristics to detect if interference has happened,
I think there is a need for something like a hook_base_seq per-netns
that can be used:

        cb->seq = READ_ONCE(...hook_base_seq);

which should be bumped when updating the hook arrays via WRITE_ONCE().

Then, a call at the end of the dump:

        nl_dump_check_consistent(cb, nlmsg_hdr(skb));

will set on NLM_F_DUMP_INTR flag so userspace can retry.

I would suggest to check that i is safe to access the array, ie.

        if (i >= e->num_hook_entries)
                goto out;

and perform the netlink dump even if it will result in a stale dump,
which is what other subsystems do, ie. just add a safety check here.

We can discuss later on how to make improvements to the netlink dump
logic to detect an interference early.

Thanks.

On Fri, Jul 10, 2026 at 05:14:07PM +0200, Phil Sutter wrote:
> Changes since v2:
> - Drop patch 1 again. Fixing nfnl_hook_dump's broken NLM_F_DUMP_INTR
>   flag setting in corner-cases is out of scope of this series and should
>   be solved in a more elegant way in nfnetlink itself.
> Changes since v1:
> - New patch 1
> - Fixed three aspects of patch 3
> - Fixed two issues of patch 5
> 
> Patch 1 is mere preparation to add the missing case handling to
> nfnl_hook_dump_nat().
> 
> Patch 2 fixes for missing READ_ONCE() calls when addressing hook ops
> array elements.
> 
> Patch 3 adds missing multipart dump support to nfnl_hook_dump_nat.
> 
> Patch 4 adds code to detect and mitigate concurrent hook updates while
> amidst a multipart dump to nfnl_hook_dump_nat.
> 
> 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: Address hook ops 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 | 48 ++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> -- 
> 2.54.0
> 

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

[Pablo Neira Ayuso]

On Fri, Jul 24, 2026 at 12:32:17PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> I think we have to tackle this from a different angle the
> nfnl_hook_dump_nat() dump issues, see:

Another option is: I take this as is and I follow up myself on top,
this is already improving the situation.

Let me know, thanks!