* [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps
@ 2026-09-04 12:52 Phil Sutter
2026-09-07 18:19 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2026-09-04 12:52 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Handling of concurrent hook changes with a dump in progress was
problematic in nfnl_hook_dump and entirely broken in nfnl_hook_dump_nat.
Address all issues in a single patch to please the review LLM.
Introduce sequence numbers in struct netns_nf to replace pointer
value-based modification detection which may fail due to memory buffer
reuse. This also eliminates the need for most manual cb->seq updates and
excessive array index value checks.
Since nat hook updates are protected by a different mutex than others,
they have to bump their own sequence number. nfnl_hook_dump_nat
therefore open-codes what nl_dump_check_consistent does but bumps
cb->seq instead of setting NLM_F_DUMP_INTR flag.
Dumps of many nat hooks was entirely broken since the array index was
not (re)stored. Use cb->args[1] for that and make sure it is reset upon
completion as the same dump may loop over multiple arrays of nat hooks.
In nfnl_hook_dump, don't signal NLM_F_DUMP_INTR if
nfnl_hook_entries_head returns error: This is a permanent condition
which does not change during a dump. It is already caught by
nfnl_hook_dump_start though, so should not happen anyway.
In general, access ops array pointer values using READ_ONCE since they
are assigned to using WRITE_ONCE.
Avoid setting NLM_F_DUMP_INTR flag in garbage memory by calling
nl_dump_check_consistent only for non-empty skbs. If not a single
netlink message was created, nfnetlink code will take care of setting
the flag.
Fixes: e2cf17d3774c ("netfilter: add new hook nfnl subsystem")
Fixes: b010e2a4a9ac ("netfilter: nfnetlink_hook: Dump nat type chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v2:
- Bump seq after RCU-pointer assign in __nf_register_net_hook
- Fix coding style (braces)
- Rebase onto https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260902232856.1024959-1-pablo@netfilter.org/
Changes since v1:
- Reset cursor value and perform EINTR check if NAT priv->entries
becomes NULL during dump.
---
include/net/netns/netfilter.h | 2 +
net/netfilter/core.c | 13 ++++++
net/netfilter/nf_nat_core.c | 11 +++++
net/netfilter/nfnetlink_hook.c | 74 ++++++++++++++++++++--------------
4 files changed, 69 insertions(+), 31 deletions(-)
diff --git a/include/net/netns/netfilter.h b/include/net/netns/netfilter.h
index a6a0bf4a247e..7fd78394d1e7 100644
--- a/include/net/netns/netfilter.h
+++ b/include/net/netns/netfilter.h
@@ -33,5 +33,7 @@ struct netns_nf {
#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
unsigned int defrag_ipv6_users;
#endif
+ unsigned int hook_base_seq;
+ unsigned int nat_hook_base_seq;
};
#endif
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 675a1034b340..d041f916c98b 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -386,6 +386,15 @@ static void nf_static_key_dec(const struct nf_hook_ops *reg, int pf)
#endif
}
+static void bump_hook_base_seq(struct net *net)
+{
+ unsigned int base_seq = READ_ONCE(net->nf.hook_base_seq);
+
+ while (++base_seq == 0)
+ ;
+ smp_store_release(&net->nf.hook_base_seq, base_seq);
+}
+
static int __nf_register_net_hook(struct net *net, int pf,
const struct nf_hook_ops *reg)
{
@@ -430,6 +439,7 @@ static int __nf_register_net_hook(struct net *net, int pf,
if (!IS_ERR(new_hooks)) {
hooks_validate(new_hooks);
rcu_assign_pointer(*pp, new_hooks);
+ bump_hook_base_seq(net);
}
mutex_unlock(&nf_hook_mutex);
@@ -506,6 +516,7 @@ static void __nf_unregister_net_hook(struct net *net, int pf,
net_dec_egress_queue();
#endif
nf_static_key_dec(reg, pf);
+ bump_hook_base_seq(net);
} else {
WARN_ONCE(1, "hook not found, pf %d num %d", pf, reg->hooknum);
}
@@ -784,6 +795,8 @@ static int __net_init netfilter_net_init(struct net *net)
return -ENOMEM;
}
#endif
+ net->nf.hook_base_seq = 1;
+ net->nf.nat_hook_base_seq = 1;
return 0;
}
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index a4858c2b2d65..27d67a677aa4 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -1160,6 +1160,15 @@ nfnetlink_parse_nat_setup(struct nf_conn *ct,
}
#endif
+static void bump_nat_hook_base_seq(struct net *net)
+{
+ unsigned int base_seq = READ_ONCE(net->nf.nat_hook_base_seq);
+
+ while (++base_seq == 0)
+ ;
+ smp_store_release(&net->nf.nat_hook_base_seq, base_seq);
+}
+
static struct nf_ct_helper_expectfn follow_master_nat = {
.name = "nat-follow-master",
.expectfn = nf_nat_follow_master,
@@ -1244,6 +1253,7 @@ int nf_nat_register_fn(struct net *net, u8 pf, const struct nf_hook_ops *ops,
nat_proto_net->nat_hook_ops = nat_ops;
nat_proto_net->users++;
+ bump_nat_hook_base_seq(net);
mutex_unlock(&nf_nat_proto_mutex);
@@ -1298,6 +1308,7 @@ void nf_nat_unregister_fn(struct net *net, u8 pf, const struct nf_hook_ops *ops,
goto unlock;
priv = nat_ops[hooknum].priv;
nf_hook_entries_delete_raw(&priv->entries, ops);
+ bump_nat_hook_base_seq(net);
if (nat_proto_net->users == 0) {
nf_unregister_net_hooks(net, nat_ops, ops_count);
diff --git a/net/netfilter/nfnetlink_hook.c b/net/netfilter/nfnetlink_hook.c
index 95005e9a6066..b05fd79397c5 100644
--- a/net/netfilter/nfnetlink_hook.c
+++ b/net/netfilter/nfnetlink_hook.c
@@ -54,7 +54,6 @@ 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;
u8 hook;
};
@@ -338,27 +337,47 @@ 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 net *net = sock_net(nlskb->sk);
struct nf_hook_ops **nat_ops;
- int i, err;
+ unsigned int i = cb->args[1];
+ struct nf_hook_entries *e;
+ unsigned int base_seq;
+ int err = 0;
+ base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
+
+ e = rcu_dereference(priv->entries);
if (!e)
- return 0;
+ goto out;
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],
- ops->priority, family, seq);
+ 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;
+out:
+ if (!err)
+ i = 0;
+ cb->args[1] = i;
+
+ if (cb->args[2] && base_seq != cb->args[2]) {
+ cb->seq++;
+ err = -EINTR;
+ }
+ cb->args[2] = base_seq;
+
+ return err;
}
static int nfnl_hook_dump(struct sk_buff *nlskb,
@@ -373,35 +392,31 @@ static int nfnl_hook_dump(struct sk_buff *nlskb,
unsigned int i = cb->args[0];
rcu_read_lock();
+ cb->seq = smp_load_acquire(&net->nf.hook_base_seq);
e = nfnl_hook_entries_head(family, ctx->hook, net, ctx->devname);
- if (!e)
+ if (!e || IS_ERR(e))
goto done;
- if (IS_ERR(e)) {
- cb->seq++;
- goto done;
- }
-
- if ((unsigned long)e != ctx->headv || i >= e->num_hook_entries)
- cb->seq++;
-
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, ctx, ops[i], family,
- cb->nlh->nlmsg_seq);
- else
- err = nfnl_hook_dump_one(nlskb, ctx, ops[i],
- ops[i]->priority, 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, cur,
+ cur->priority, family,
cb->nlh->nlmsg_seq);
+ }
if (err)
break;
}
done:
- nl_dump_check_consistent(cb, nlmsg_hdr(nlskb));
+ if (nlskb->len > 0)
+ nl_dump_check_consistent(cb, nlmsg_hdr(nlskb));
rcu_read_unlock();
cb->args[0] = i;
return nlskb->len;
@@ -442,10 +457,7 @@ static int nfnl_hook_dump_start(struct netlink_callback *cb)
return -ENOMEM;
strscpy(ctx->devname, name, sizeof(ctx->devname));
- ctx->headv = (unsigned long)head;
ctx->hook = hooknum;
-
- cb->seq = 1;
cb->data = ctx;
return 0;
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps
2026-09-04 12:52 [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps Phil Sutter
@ 2026-09-07 18:19 ` Pablo Neira Ayuso
2026-09-08 9:23 ` Phil Sutter
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2026-09-07 18:19 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel
Hi Phil,
On Fri, Sep 04, 2026 at 02:52:15PM +0200, Phil Sutter wrote:
> @@ -338,27 +337,47 @@ 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 net *net = sock_net(nlskb->sk);
> struct nf_hook_ops **nat_ops;
> - int i, err;
> + unsigned int i = cb->args[1];
> + struct nf_hook_entries *e;
> + unsigned int base_seq;
> + int err = 0;
>
> + base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
Maybe annnotate this base sequence in the .start via:
struct netlink_dump_control c = {
.start = ...;
We should probably start doing this in other nfnetlink subsystems too.
This will help catch an interference between two netlink recv() calls
which results in calling netlink_dump() which calls this function.
Let me know, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps
2026-09-07 18:19 ` Pablo Neira Ayuso
@ 2026-09-08 9:23 ` Phil Sutter
2026-09-08 12:41 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2026-09-08 9:23 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Hi Pablo,
On Mon, Sep 07, 2026 at 08:19:47PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Sep 04, 2026 at 02:52:15PM +0200, Phil Sutter wrote:
> > @@ -338,27 +337,47 @@ 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 net *net = sock_net(nlskb->sk);
> > struct nf_hook_ops **nat_ops;
> > - int i, err;
> > + unsigned int i = cb->args[1];
> > + struct nf_hook_entries *e;
> > + unsigned int base_seq;
> > + int err = 0;
> >
> > + base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
>
> Maybe annnotate this base sequence in the .start via:
>
> struct netlink_dump_control c = {
> .start = ...;
>
> We should probably start doing this in other nfnetlink subsystems too.
>
> This will help catch an interference between two netlink recv() calls
> which results in calling netlink_dump() which calls this function.
I do not comprehend, sorry. The concurrent hook dumps have distinct cb
buffers and net->nf.{nat_,}hook_base_seq is shared but read-only. How
does the problematic interference happen?
Cheers, Phil
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps
2026-09-08 9:23 ` Phil Sutter
@ 2026-09-08 12:41 ` Pablo Neira Ayuso
2026-09-08 13:57 ` Phil Sutter
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2026-09-08 12:41 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel
On Tue, Sep 08, 2026 at 11:23:03AM +0200, Phil Sutter wrote:
> Hi Pablo,
>
> On Mon, Sep 07, 2026 at 08:19:47PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Sep 04, 2026 at 02:52:15PM +0200, Phil Sutter wrote:
> > > @@ -338,27 +337,47 @@ 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 net *net = sock_net(nlskb->sk);
> > > struct nf_hook_ops **nat_ops;
> > > - int i, err;
> > > + unsigned int i = cb->args[1];
> > > + struct nf_hook_entries *e;
> > > + unsigned int base_seq;
> > > + int err = 0;
> > >
> > > + base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
> >
> > Maybe annnotate this base sequence in the .start via:
> >
> > struct netlink_dump_control c = {
> > .start = ...;
> >
> > We should probably start doing this in other nfnetlink subsystems too.
> >
> > This will help catch an interference between two netlink recv() calls
> > which results in calling netlink_dump() which calls this function.
>
> I do not comprehend, sorry. The concurrent hook dumps have distinct cb
> buffers and net->nf.{nat_,}hook_base_seq is shared but read-only. How
> does the problematic interference happen?
See nf_tables_dump_rules_start() for instance. It allocates the struct
nft_rule_dump_ctx, which is reachable through .start, .dump and .done
callbacks. I think it should be possible to annotate the current
base_seq at the beginning of the netlink dump from .start in a ctx
object. Then, use it from .dump to check if dump is consistent (ie.
turn on the NLM_F_DUMP_INTR flag).
So, instead of fetching the current sequence from .dump like this:
cb->seq = nft_base_seq(net);
Use the sequence available in the new ctx object, ie. from .dump path
you do this:
cb->seq = ctx->seq;
Because netlink_dump() is called for each userspace recv() call (netlink
delivers the chunked listing in several messages), this would allow
userspace to know that the listing is inconsistent, then optionally
retry.
Makes sense to you?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps
2026-09-08 12:41 ` Pablo Neira Ayuso
@ 2026-09-08 13:57 ` Phil Sutter
2026-09-09 0:34 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2026-09-08 13:57 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On Tue, Sep 08, 2026 at 02:41:05PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Sep 08, 2026 at 11:23:03AM +0200, Phil Sutter wrote:
> > Hi Pablo,
> >
> > On Mon, Sep 07, 2026 at 08:19:47PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Sep 04, 2026 at 02:52:15PM +0200, Phil Sutter wrote:
> > > > @@ -338,27 +337,47 @@ 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 net *net = sock_net(nlskb->sk);
> > > > struct nf_hook_ops **nat_ops;
> > > > - int i, err;
> > > > + unsigned int i = cb->args[1];
> > > > + struct nf_hook_entries *e;
> > > > + unsigned int base_seq;
> > > > + int err = 0;
> > > >
> > > > + base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
> > >
> > > Maybe annnotate this base sequence in the .start via:
> > >
> > > struct netlink_dump_control c = {
> > > .start = ...;
> > >
> > > We should probably start doing this in other nfnetlink subsystems too.
> > >
> > > This will help catch an interference between two netlink recv() calls
> > > which results in calling netlink_dump() which calls this function.
> >
> > I do not comprehend, sorry. The concurrent hook dumps have distinct cb
> > buffers and net->nf.{nat_,}hook_base_seq is shared but read-only. How
> > does the problematic interference happen?
>
> See nf_tables_dump_rules_start() for instance. It allocates the struct
> nft_rule_dump_ctx, which is reachable through .start, .dump and .done
> callbacks. I think it should be possible to annotate the current
> base_seq at the beginning of the netlink dump from .start in a ctx
> object. Then, use it from .dump to check if dump is consistent (ie.
> turn on the NLM_F_DUMP_INTR flag).
>
> So, instead of fetching the current sequence from .dump like this:
>
> cb->seq = nft_base_seq(net);
>
> Use the sequence available in the new ctx object, ie. from .dump path
> you do this:
>
> cb->seq = ctx->seq;
But to detect a concurrent hook update between to .dump callback calls
(if skb space was exceeded), the current value in per-net data has to
be fetched, no? So this would have to look like this in .dump callback:
| ctx->seq = nft_base_seq(net);
| cb->seq = ctx->seq;
Then I don't get the detour via struct nfnl_dump_hook_data. Or is it
possible we get multiple concurrent dump requests on the same netlink
socket and thus cb->seq (and cb->prev_seq) gets shared between them?
> Because netlink_dump() is called for each userspace recv() call (netlink
> delivers the chunked listing in several messages), this would allow
> userspace to know that the listing is inconsistent, then optionally
> retry.
>
> Makes sense to you?
Not quite, sorry.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps
2026-09-08 13:57 ` Phil Sutter
@ 2026-09-09 0:34 ` Pablo Neira Ayuso
0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2026-09-09 0:34 UTC (permalink / raw)
To: Phil Sutter; +Cc: netfilter-devel
On Tue, Sep 08, 2026 at 03:57:03PM +0200, Phil Sutter wrote:
> On Tue, Sep 08, 2026 at 02:41:05PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Sep 08, 2026 at 11:23:03AM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > >
> > > On Mon, Sep 07, 2026 at 08:19:47PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Sep 04, 2026 at 02:52:15PM +0200, Phil Sutter wrote:
> > > > > @@ -338,27 +337,47 @@ 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 net *net = sock_net(nlskb->sk);
> > > > > struct nf_hook_ops **nat_ops;
> > > > > - int i, err;
> > > > > + unsigned int i = cb->args[1];
> > > > > + struct nf_hook_entries *e;
> > > > > + unsigned int base_seq;
> > > > > + int err = 0;
> > > > >
> > > > > + base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
> > > >
> > > > Maybe annnotate this base sequence in the .start via:
> > > >
> > > > struct netlink_dump_control c = {
> > > > .start = ...;
> > > >
> > > > We should probably start doing this in other nfnetlink subsystems too.
> > > >
> > > > This will help catch an interference between two netlink recv() calls
> > > > which results in calling netlink_dump() which calls this function.
> > >
> > > I do not comprehend, sorry. The concurrent hook dumps have distinct cb
> > > buffers and net->nf.{nat_,}hook_base_seq is shared but read-only. How
> > > does the problematic interference happen?
> >
> > See nf_tables_dump_rules_start() for instance. It allocates the struct
> > nft_rule_dump_ctx, which is reachable through .start, .dump and .done
> > callbacks. I think it should be possible to annotate the current
> > base_seq at the beginning of the netlink dump from .start in a ctx
> > object. Then, use it from .dump to check if dump is consistent (ie.
> > turn on the NLM_F_DUMP_INTR flag).
> >
> > So, instead of fetching the current sequence from .dump like this:
> >
> > cb->seq = nft_base_seq(net);
> >
> > Use the sequence available in the new ctx object, ie. from .dump path
> > you do this:
> >
> > cb->seq = ctx->seq;
>
> But to detect a concurrent hook update between to .dump callback calls
> (if skb space was exceeded), the current value in per-net data has to
> be fetched, no? So this would have to look like this in .dump callback:
>
> | ctx->seq = nft_base_seq(net);
> | cb->seq = ctx->seq;
>
> Then I don't get the detour via struct nfnl_dump_hook_data. Or is it
> possible we get multiple concurrent dump requests on the same netlink
> socket and thus cb->seq (and cb->prev_seq) gets shared between them?
>
> > Because netlink_dump() is called for each userspace recv() call (netlink
> > delivers the chunked listing in several messages), this would allow
> > userspace to know that the listing is inconsistent, then optionally
> > retry.
> >
> > Makes sense to you?
>
> Not quite, sorry.
Right, I got confused by the LLM report.
Should be postpone bumping the seq also after array has been shrunk as
LLM suggest in the remove path? For consistency with the insert
operations, just bump sequence _after_ the datastructure update.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-09 0:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-04 12:52 [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps Phil Sutter
2026-09-07 18:19 ` Pablo Neira Ayuso
2026-09-08 9:23 ` Phil Sutter
2026-09-08 12:41 ` Pablo Neira Ayuso
2026-09-08 13:57 ` Phil Sutter
2026-09-09 0:34 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox