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

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

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
>