Re: [PATCH nf,v2 3/3] netfilter: nf_tables: add hook transactions for device deletions

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

On Wed, Apr 15, 2026 at 10:51:09PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Restore the flag that indicates that the hook is going away, ie.
> > NFT_HOOK_REMOVE, but add a new transaction object to track deletion
> > of hooks without altering the basechain/flowtable hook_list during
> > the preparation phase.
> > 
> > The existing approach that moves the hook from the basechain/flowtable
> > hook_list to transaction hook_list breaks netlink dump path readers
> > of this RCU-protected list.
> > 
> > It should be possible use an array for nft_trans_hook to store the
> > deleted hooks to compact the representation but I am not expecting
> > many hook object, specially now that wildcard support for devices
> > is in place.
> > 
> > Note that the nft_trans_chain_hooks() list contains a list of struct
> > nft_trans_hook objects for DELCHAIN and DELFLOWTABLE commands, while
> > this list stores struct nft_hook objects for NEWCHAIN and NEWFLOWTABLE.
> > Note that new commands can be updated to use nft_trans_hook for
> > consistency.
> > 
> > Fixes: 7d937b107108 ("netfilter: nf_tables: support for deleting devices in an existing netdev chain")
> > Fixes: b6d9014a3335 ("netfilter: nf_tables: delete flowtable hooks via transaction list")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > v2: no changes.
> > 
> >  include/net/netfilter/nf_tables.h |  13 ++++
> >  net/netfilter/nf_tables_api.c     | 124 ++++++++++++++++++++++++++----
> >  2 files changed, 120 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> > index ec8a8ec9c0aa..3ec41574af77 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -1216,12 +1216,15 @@ struct nft_stats {
> >  	struct u64_stats_sync	syncp;
> >  };
> >  
> > +#define NFT_HOOK_REMOVE	(1 << 0)
> > +
> >  struct nft_hook {
> >  	struct list_head	list;
> >  	struct list_head	ops_list;
> >  	struct rcu_head		rcu;
> >  	char			ifname[IFNAMSIZ];
> >  	u8			ifnamelen;
> > +	u8			flags;
> >  };
> >  
> >  struct nf_hook_ops *nft_hook_find_ops(const struct nft_hook *hook,
> > @@ -1676,6 +1679,16 @@ struct nft_trans {
> >  	u8				put_net:1;
> >  };
> >  
> > +/**
> > + * struct nft_trans_hook - nf_tables hook update in transaction
> > + * @list: used internally
> > + * @hook: struct nft_hook with the device hook
> > + */
> > +struct nft_trans_hook {
> > +	struct list_head		list;
> > +	struct nft_hook			*hook;
> > +};
> 
> Do I get this correctly?
> 
> nft_trans_container_flowtable(trans)->hook_list
> and
> nft_trans_container_chain(trans)->hook_list
> 
> Either hold 'struct nft_hook' objects or nft_trans_hook objects?
> Former when adding, latter when removing from existing base hook?

Add, update -> struct nft_hook
Delete -> struct nft_trans_hook

Yes. I could add a separated list, but this list is exclusive for the
transaction object. Another option is a union to highlight how it is
used, but it is not better than the current mixed semantics, which are
not ideal.

As a follow up, it should be possible to use nft_trans_hook for
updates too in nf-next for consistency.

> > +		trans_hook = kmalloc(sizeof(*trans_hook), GFP_KERNEL);
> 
> Note that 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
> transformed such allocation requests to use "kmalloc_obj(*trans_hook, GFP_KERNEL);"
> instead.

I will replace it to use the new kmalloc_obj().