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))
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;
unsigned int len;
struct cmd *cmd;
diff --git a/include/proto.h b/include/proto.h
index 3a20ff8c4071..3756a4ab79a4 100644
--- a/include/proto.h
+++ b/include/proto.h
@@ -13,6 +13,7 @@
* @PROTO_BASE_NETWORK_HDR: network layer header
* @PROTO_BASE_TRANSPORT_HDR: transport layer header
*/
+__attribute__((packed))
enum proto_bases {
PROTO_BASE_INVALID,
PROTO_BASE_LL_HDR,
@@ -26,6 +27,7 @@ enum proto_bases {
extern const char *proto_base_names[];
extern const char *proto_base_tokens[];
+__attribute__((packed))
enum icmp_hdr_field_type {
PROTO_ICMP_ANY = 0,
PROTO_ICMP_ECHO, /* echo and reply */
@@ -52,9 +54,9 @@ struct proto_hdr_template {
const struct datatype *dtype;
uint16_t offset;
uint16_t len;
- enum byteorder byteorder:8;
+ enum byteorder byteorder;
enum nft_meta_keys meta_key:8;
- enum icmp_hdr_field_type icmp_dep:8;
+ enum icmp_hdr_field_type icmp_dep;
};
#define PROTO_HDR_TEMPLATE(__token, __dtype, __byteorder, __offset, __len)\
@@ -77,6 +79,7 @@ struct proto_hdr_template {
#define PROTO_UPPER_MAX 16
#define PROTO_HDRS_MAX 20
+__attribute__((packed))
enum proto_desc_id {
PROTO_DESC_UNKNOWN = 0,
PROTO_DESC_AH,
@@ -119,8 +122,8 @@ enum proto_desc_id {
*/
struct proto_desc {
const char *name;
- enum proto_desc_id id:8;
- enum proto_bases base:8;
+ enum proto_desc_id id;
+ enum proto_bases base;
enum nft_payload_csum_types checksum_type:8;
uint16_t checksum_key;
uint16_t protocol_key;
--
2.41.0
