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@xxxxxxxxxx>
> ---
> 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.
