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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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
>