Re: [PATCH nft 8/9] datatype: use __attribute__((packed)) instead of enum bitfields

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

On Wed, Sep 20, 2023 at 04:26:09PM +0200, Thomas Haller wrote:
> At some places we use bitfields of those enums, to save space inside the
> structure. We can achieve that in a better way, by using GCC's
> __attribute__((packed)) on the enum type.
> 
> It's better because a :8 bitfield makes the assumption that all enum
> values (valid or invalid) fit into that field. With packed enums, we
> don't need that assumption as the field can hold all possible numbers
> that the enum type can hold. This reduces the places we need to worry
> about truncating a value to casts between other types and the enum.
> Those places already require us to be careful.
> 
> On the other hand, previously casting an int (or uint32_t) likely didn't
> cause a truncation as the underlying type was large enough. So we could
> check for invalid enum values after the cast. We might do that at
> places. For example, we do
> 
> 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_META_KEY);
> 	expr = meta_expr_alloc(loc, key);
> 
> where we cast from an uint32_t to an enum without checking. Note that
> `enum nft_meta_keys` is not packed by this patch. But this is an example
> how things could be wrong. But the bug already exits before: don't make
> assumption about the underlying enum type and take care of truncation
> during casts.
> 
> This makes the change potentially dangerous, and it's hard to be sure
> that it doesn't uncover bugs (due tow rong assumptions about enum types).
> 
> Note that we were already using the GCC-ism __attribute__((packed))
> previously, however on a struct and not on an enum. Anyway. It seems
> unlikely that we support any other compilers than GCC/Clang. Those both
> support this attribute.  We should not worry about portability towards
> hypothetical compilers (the C standard), unless there is a real compiler
> that we can use and test and shows a problem with this. Especially when
> we support both GCC and Clang, which themselves are ubiquitous and
> accessible to all users (as they also need to build the kernel in the
> first place).
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/datatype.h   |  1 +
>  include/expression.h |  8 +++++---
>  include/proto.h      | 11 +++++++----
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/include/datatype.h b/include/datatype.h
> index 202935bd322f..c8b3b77ad0c0 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -112,6 +112,7 @@ enum datatypes {
>   * @BYTEORDER_HOST_ENDIAN:	host endian
>   * @BYTEORDER_BIG_ENDIAN:	big endian
>   */
> +__attribute__((packed))
>  enum byteorder {
>  	BYTEORDER_INVALID,
>  	BYTEORDER_HOST_ENDIAN,
> diff --git a/include/expression.h b/include/expression.h
> index aede223db741..11a1dbf00b8c 100644
> --- a/include/expression.h
> +++ b/include/expression.h
> @@ -45,6 +45,7 @@
>   * @EXPR_SET_ELEM_CATCHALL catchall element expression
>   * @EXPR_FLAGCMP	flagcmp expression
>   */
> +__attribute__((packed))

No need for ((packed)) here, we never send this over the wire.

>  enum expr_types {
>  	EXPR_INVALID,
>  	EXPR_VERDICT,
> @@ -80,6 +81,7 @@ enum expr_types {
>  	EXPR_MAX = EXPR_FLAGCMP
>  };
>  
> +__attribute__((packed))
>  enum ops {
>  	OP_INVALID,
>  	OP_IMPLICIT,
> @@ -247,9 +249,9 @@ struct expr {
>  	unsigned int		flags;
>  
>  	const struct datatype	*dtype;
> -	enum byteorder		byteorder:8;
> -	enum expr_types		etype:8;
> -	enum ops		op:8;
> +	enum byteorder		byteorder;
> +	enum expr_types		etype;
> +	enum ops		op;

This is saving _a lot of space_ for us, we currently have a problem
with memory consumption, this is going in the opposite direction.

I prefer to ditch this patch.