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

[Florian Westphal <fw@strlen.de>]

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?

> +		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.