Re: [PATCH nf-next 01/11] netfilter: nf_tables: make struct nft_trans first member of derived subtypes

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

On Mon, May 13, 2024 at 03:00:41PM +0200, Florian Westphal wrote:
> There is 'struct nft_trans', the basic structure for all transactional
> objects, and the the various different transactional objects, such as
> nft_trans_table, chain, set, set_elem and so on.
> 
> Right now 'struct nft_trans' uses a flexible member at the tail
> (data[]), and casting is needed to access the actual type-specific
> members.
> 
> Change this to make the hierarchy visible in source code, i.e. make
> struct nft_trans the first member of all derived subtypes.
> 
> This has several advantages:
> 1. pahole output reflects the real size needed by the particular subtype
> 2. allows to use container_of() to convert the base type to the actual
>    object type instead of casting ->data to the overlay structure.
> 3. It makes it easy to add intermediate types.
> 
> 'struct nft_trans' contains a 'binding_list' that is only needed
> by two subtypes, so it should be part of the two subtypes, not in
> the base structure.
> 
> But that makes it hard to interate over the binding_list, because
> there is no common base structure.
> 
> A follow patch moves the bind list to a new struct:
> 
>  struct nft_trans_binding {
>    struct nft_trans nft_trans;
>    struct list_head binding_list;
>  };
> 
> ... and makes that structure the new 'first member' for both
> nft_trans_chain and nft_trans_set.
> 
> No functional change intended in this patch.
> 
> Some numbers:
>  struct nft_trans { /* size: 88, cachelines: 2, members: 5 */
>  struct nft_trans_chain { /* size: 152, cachelines: 3, members: 10 */
>  struct nft_trans_elem { /* size: 112, cachelines: 2, members: 4 */
>  struct nft_trans_flowtable { /* size: 128, cachelines: 2, members: 5 */
>  struct nft_trans_obj { /* size: 112, cachelines: 2, members: 4 */
>  struct nft_trans_rule { /* size: 112, cachelines: 2, members: 5 */
>  struct nft_trans_set { /* size: 120, cachelines: 2, members: 8 */
>  struct nft_trans_table { /* size: 96, cachelines: 2, members: 2 */
> 
> Of particular interest is nft_trans_elem, which needs to be allocated
> once for each pending (to be added or removed) set element.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/net/netfilter/nf_tables.h | 88 +++++++++++++++++--------------
>  net/netfilter/nf_tables_api.c     | 10 ++--
>  2 files changed, 54 insertions(+), 44 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 2796153b03da..af0ef21f3780 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1608,14 +1608,16 @@ static inline int nft_set_elem_is_dead(const struct nft_set_ext *ext)
>  }
>  
>  /**
> - *	struct nft_trans - nf_tables object update in transaction
> + * struct nft_trans - nf_tables object update in transaction
>   *
> - *	@list: used internally
> - *	@binding_list: list of objects with possible bindings
> - *	@msg_type: message type
> - *	@put_net: ctx->net needs to be put
> - *	@ctx: transaction context
> - *	@data: internal information related to the transaction
> + * @list: used internally
> + * @binding_list: list of objects with possible bindings
> + * @msg_type: message type
> + * @put_net: ctx->net needs to be put
> + * @ctx: transaction context
> + *
> + * This is the information common to all objects in the transaction,
> + * this must always be the first member of derived sub-types.
>   */
>  struct nft_trans {
>  	struct list_head		list;
> @@ -1623,10 +1625,10 @@ struct nft_trans {
>  	int				msg_type;
>  	bool				put_net;
>  	struct nft_ctx			ctx;
> -	char				data[];
>  };
>  
>  struct nft_trans_rule {
> +	struct nft_trans		nft_trans;
>  	struct nft_rule			*rule;
>  	struct nft_flow_rule		*flow;
>  	u32				rule_id;
> @@ -1634,15 +1636,16 @@ struct nft_trans_rule {
>  };
>  
>  #define nft_trans_rule(trans)	\
> -	(((struct nft_trans_rule *)trans->data)->rule)
> +	container_of(trans, struct nft_trans_rule, nft_trans)->rule

Another nitpicking comestic issue:

Maybe I can get this series slightly smaller if

        nft_trans_rule_container

is added here and use it, instead of the opencoded container_of.

For consistency with:

#define nft_trans_container_set(t)

which is used everywhere in this header in a follow up patch.

I mangle this at the expense of adding my own bugs :)

[...]
>  #define nft_trans_set(trans)	\
> -	(((struct nft_trans_set *)trans->data)->set)
> +	container_of(trans, struct nft_trans_set, nft_trans)->set
>  #define nft_trans_set_id(trans)	\
> -	(((struct nft_trans_set *)trans->data)->set_id)
> +	container_of(trans, struct nft_trans_set, nft_trans)->set_id
>  #define nft_trans_set_bound(trans)	\
> -	(((struct nft_trans_set *)trans->data)->bound)
> +	container_of(trans, struct nft_trans_set, nft_trans)->bound
>  #define nft_trans_set_update(trans)	\
> -	(((struct nft_trans_set *)trans->data)->update)
> +	container_of(trans, struct nft_trans_set, nft_trans)->update
>  #define nft_trans_set_timeout(trans)	\
> -	(((struct nft_trans_set *)trans->data)->timeout)
> +	container_of(trans, struct nft_trans_set, nft_trans)->timeout
>  #define nft_trans_set_gc_int(trans)	\
> -	(((struct nft_trans_set *)trans->data)->gc_int)
> +	container_of(trans, struct nft_trans_set, nft_trans)->gc_int
>  #define nft_trans_set_size(trans)	\
> -	(((struct nft_trans_set *)trans->data)->size)
> +	container_of(trans, struct nft_trans_set, nft_trans)->size