[PATCH nft 0/9] various cleanups related to enums and struct datatype

[PATCH nft 0/9] various cleanups related to enums and struct datatype

[Thomas Haller]

Various loosely related patches in the hope to improve something.

Thomas Haller (9):
  src: fix indentation/whitespace
  include: fix missing definitions in <cache.h>/<headers.h>
  datatype: drop flags field from datatype
  datatype: use "enum byteorder" instead of int in set_datatype_alloc()
  payload: use enum icmp_hdr_field_type in
    payload_may_dependency_kill_icmp()
  netlink: handle invalid etype in set_make_key()
  expression: cleanup expr_ops_by_type() and handle u32 input
  datatype: use __attribute__((packed)) instead of enum bitfields
  proto: add missing proto_definitions for PROTO_DESC_GENEVE

 include/cache.h           |  9 +++++++++
 include/datatype.h        | 27 +++++++++++----------------
 include/expression.h      | 10 ++++++----
 include/headers.h         |  2 ++
 include/proto.h           | 11 +++++++----
 src/datatype.c            | 22 +++++++++-------------
 src/evaluate.c            |  2 +-
 src/expression.c          | 23 +++++++++++------------
 src/meta.c                |  6 +++---
 src/netlink.c             |  6 ++++--
 src/netlink_delinearize.c |  2 +-
 src/payload.c             | 10 ++++------
 src/proto.c               |  3 ++-
 src/rt.c                  |  2 +-
 src/segtree.c             |  5 ++---
 15 files changed, 73 insertions(+), 67 deletions(-)

-- 
2.41.0

[PATCH nft 1/9] src: fix indentation/whitespace

[Thomas Haller]

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/meta.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/meta.c b/src/meta.c
index d8fc5f585e74..181e111cbbdc 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -968,8 +968,8 @@ struct stmt *meta_stmt_alloc(const struct location *loc, enum nft_meta_keys key,
 	stmt->meta.key	= key;
 	stmt->meta.expr	= expr;
 
-        if (key < array_size(meta_templates))
-                stmt->meta.tmpl = &meta_templates[key];
+	if (key < array_size(meta_templates))
+		stmt->meta.tmpl = &meta_templates[key];
 
 	return stmt;
 }
-- 
2.41.0

Re: [PATCH nft 1/9] src: fix indentation/whitespace

[Pablo Neira Ayuso]

Applied, thanks

[PATCH nft 2/9] include: fix missing definitions in <cache.h>/<headers.h>

[Thomas Haller]

The headers should be self-contained so they can be included in any
order. With exception of <nft.h>, which any internal header can rely on.

Some fixes for <cache.h>/<headers.h>.

In case of <cache.h>, forward declare some of the structs instead of
including the headers. <headers.h> uses struct in6_addr.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/cache.h   | 9 +++++++++
 include/headers.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/include/cache.h b/include/cache.h
index 934c3a74fa95..e66b0af5fe0f 100644
--- a/include/cache.h
+++ b/include/cache.h
@@ -3,6 +3,8 @@
 
 #include <string.h>
 
+#include <list.h>
+
 struct handle;
 
 enum cache_level_bits {
@@ -66,6 +68,7 @@ struct nft_cache_filter {
 };
 
 struct nft_cache;
+struct nft_ctx;
 enum cmd_ops;
 
 int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
@@ -97,6 +100,8 @@ void chain_cache_add(struct chain *chain, struct table *table);
 void chain_cache_del(struct chain *chain);
 struct chain *chain_cache_find(const struct table *table, const char *name);
 
+struct set;
+
 void set_cache_add(struct set *set, struct table *table);
 void set_cache_del(struct set *set);
 struct set *set_cache_find(const struct table *table, const char *name);
@@ -121,6 +126,8 @@ void table_cache_del(struct table *table);
 struct table *table_cache_find(const struct cache *cache, const char *name,
 			       uint32_t family);
 
+struct obj;
+
 void obj_cache_add(struct obj *obj, struct table *table);
 void obj_cache_del(struct obj *obj);
 struct obj *obj_cache_find(const struct table *table, const char *name,
@@ -138,6 +145,8 @@ struct nft_cache {
 	uint32_t		flags;
 };
 
+struct netlink_ctx;
+
 void nft_chain_cache_update(struct netlink_ctx *ctx, struct table *table,
 			    const char *chain);
 
diff --git a/include/headers.h b/include/headers.h
index 759f93bf8c7a..13324c72c734 100644
--- a/include/headers.h
+++ b/include/headers.h
@@ -1,6 +1,8 @@
 #ifndef NFTABLES_HEADERS_H
 #define NFTABLES_HEADERS_H
 
+#include <netinet/in.h>
+
 #ifndef IPPROTO_UDPLITE
 # define IPPROTO_UDPLITE	136
 #endif
-- 
2.41.0

[PATCH nft 3/9] datatype: drop flags field from datatype

[Thomas Haller]

Flags are not always bad. For example, as a function argument they allow
easier extension in the future. But with datatype's "flags" argument and
enum datatype_flags there are no advantages of this approach.

- replace DTYPE_F_PREFIX with a "bool f_prefix" field. This could even
  be a bool:1 bitfield if we cared to represent the information with
  one bit only. For now it's not done because that would not help reducing
  the size of the struct, so a bitfield is less preferable.

- instead of DTYPE_F_ALLOC, use the refcnt of zero to represent static
  instances. Drop this redundant flag.

- move the integer field "refcnt" in struct datatype beside other fields
  of similar size/alignment. This makes the size of the struct by one
  pointer size smaller (on x86-64).

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/datatype.h        | 24 +++++++++---------------
 src/datatype.c            | 20 ++++++++------------
 src/meta.c                |  2 +-
 src/netlink_delinearize.c |  2 +-
 src/rt.c                  |  2 +-
 src/segtree.c             |  5 ++---
 6 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 52a2e943b252..5b85adc15857 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -120,24 +120,13 @@ enum byteorder {
 
 struct expr;
 
-/**
- * enum datatype_flags
- *
- * @DTYPE_F_ALLOC:		datatype is dynamically allocated
- * @DTYPE_F_PREFIX:		preferred representation for ranges is a prefix
- */
-enum datatype_flags {
-	DTYPE_F_ALLOC		= (1 << 0),
-	DTYPE_F_PREFIX		= (1 << 1),
-};
-
 struct parse_ctx;
 /**
  * struct datatype
  *
  * @type:	numeric identifier
  * @byteorder:	byteorder of type (non-basetypes only)
- * @flags:	flags
+ * @f_prefix:	preferred representation for ranges is a prefix
  * @size:	type size (fixed sized non-basetypes only)
  * @subtypes:	number of subtypes (concat type)
  * @name:	type name
@@ -147,14 +136,20 @@ struct parse_ctx;
  * @print:	function to print a constant of this type
  * @parse:	function to parse a symbol and return an expression
  * @sym_tbl:	symbol table for this type
- * @refcnt:	reference counter (only for DTYPE_F_ALLOC)
+ * @refcnt:	reference counter for dynamically allocated instances.
  */
 struct datatype {
 	uint32_t			type;
 	enum byteorder			byteorder;
-	unsigned int			flags;
+	bool				f_prefix;
 	unsigned int			size;
 	unsigned int			subtypes;
+
+	/* Refcount for dynamically allocated instances. For static instances
+	 * this is zero (get() and free() are NOPs).
+	 */
+	unsigned int			refcnt;
+
 	const char			*name;
 	const char			*desc;
 	const struct datatype		*basetype;
@@ -169,7 +164,6 @@ struct datatype {
 	struct error_record		*(*err)(const struct expr *sym);
 	void				(*describe)(struct output_ctx *octx);
 	const struct symbol_table	*sym_tbl;
-	unsigned int			refcnt;
 };
 
 extern const struct datatype *datatype_lookup(enum datatypes type);
diff --git a/src/datatype.c b/src/datatype.c
index 70c84846f70e..c5d88d9a90b6 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -641,7 +641,7 @@ const struct datatype ipaddr_type = {
 	.basetype	= &integer_type,
 	.print		= ipaddr_type_print,
 	.parse		= ipaddr_type_parse,
-	.flags		= DTYPE_F_PREFIX,
+	.f_prefix	= true,
 };
 
 static void ip6addr_type_print(const struct expr *expr, struct output_ctx *octx)
@@ -708,7 +708,7 @@ const struct datatype ip6addr_type = {
 	.basetype	= &integer_type,
 	.print		= ip6addr_type_print,
 	.parse		= ip6addr_type_parse,
-	.flags		= DTYPE_F_PREFIX,
+	.f_prefix	= true,
 };
 
 static void inet_protocol_type_print(const struct expr *expr,
@@ -944,7 +944,7 @@ const struct datatype mark_type = {
 	.print		= mark_type_print,
 	.json		= mark_type_json,
 	.parse		= mark_type_parse,
-	.flags		= DTYPE_F_PREFIX,
+	.f_prefix	= true,
 };
 
 static const struct symbol_table icmp_code_tbl = {
@@ -1203,9 +1203,7 @@ static struct datatype *datatype_alloc(void)
 	struct datatype *dtype;
 
 	dtype = xzalloc(sizeof(*dtype));
-	dtype->flags = DTYPE_F_ALLOC;
 	dtype->refcnt = 1;
-
 	return dtype;
 }
 
@@ -1215,10 +1213,10 @@ struct datatype *datatype_get(const struct datatype *ptr)
 
 	if (!dtype)
 		return NULL;
-	if (!(dtype->flags & DTYPE_F_ALLOC))
-		return dtype;
 
-	dtype->refcnt++;
+	if (dtype->refcnt > 0)
+		dtype->refcnt++;
+
 	return dtype;
 }
 
@@ -1245,7 +1243,6 @@ struct datatype *datatype_clone(const struct datatype *orig_dtype)
 	*dtype = *orig_dtype;
 	dtype->name = xstrdup(orig_dtype->name);
 	dtype->desc = xstrdup(orig_dtype->desc);
-	dtype->flags = DTYPE_F_ALLOC | orig_dtype->flags;
 	dtype->refcnt = 1;
 
 	return dtype;
@@ -1257,10 +1254,9 @@ void datatype_free(const struct datatype *ptr)
 
 	if (!dtype)
 		return;
-	if (!(dtype->flags & DTYPE_F_ALLOC))
-		return;
 
-	assert(dtype->refcnt != 0);
+	if (dtype->refcnt == 0)
+		return;
 
 	if (--dtype->refcnt > 0)
 		return;
diff --git a/src/meta.c b/src/meta.c
index 181e111cbbdc..7bf749b34fb4 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -368,7 +368,7 @@ const struct datatype devgroup_type = {
 	.print		= devgroup_type_print,
 	.json		= devgroup_type_json,
 	.parse		= devgroup_type_parse,
-	.flags		= DTYPE_F_PREFIX,
+	.f_prefix	= true,
 };
 
 const struct datatype ifname_type = {
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 41cb37a3ccb3..f3939be2d063 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2568,7 +2568,7 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
 		default:
 			break;
 		}
-	} else if (binop->left->dtype->flags & DTYPE_F_PREFIX &&
+	} else if (binop->left->dtype->f_prefix &&
 		   binop->op == OP_AND && expr->right->etype == EXPR_VALUE &&
 		   expr_mask_is_prefix(binop->right)) {
 		expr->left = expr_get(binop->left);
diff --git a/src/rt.c b/src/rt.c
index 9ddcb210eaad..ccea0aa9bc44 100644
--- a/src/rt.c
+++ b/src/rt.c
@@ -55,7 +55,7 @@ const struct datatype realm_type = {
 	.basetype	= &integer_type,
 	.print		= realm_type_print,
 	.parse		= realm_type_parse,
-	.flags		= DTYPE_F_PREFIX,
+	.f_prefix	= true,
 };
 
 const struct rt_template rt_templates[] = {
diff --git a/src/segtree.c b/src/segtree.c
index 0a12a0cd5151..637457b087b9 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -402,8 +402,7 @@ void concat_range_aggregate(struct expr *set)
 				goto next;
 			}
 
-			if (prefix_len < 0 ||
-			    !(r1->dtype->flags & DTYPE_F_PREFIX)) {
+			if (prefix_len < 0 || !r1->dtype->f_prefix) {
 				tmp = range_expr_alloc(&r1->location, r1,
 						       r2);
 
@@ -518,7 +517,7 @@ add_interval(struct expr *set, struct expr *low, struct expr *i)
 		expr = expr_get(low);
 	} else if (range_is_prefix(range) && !mpz_cmp_ui(p, 0)) {
 
-		if (i->dtype->flags & DTYPE_F_PREFIX)
+		if (i->dtype->f_prefix)
 			expr = interval_to_prefix(low, i, range);
 		else if (expr_basetype(i)->type == TYPE_STRING)
 			expr = interval_to_string(low, i, range);
-- 
2.41.0

Re: [PATCH nft 3/9] datatype: drop flags field from datatype

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> Flags are not always bad. For example, as a function argument they allow
> easier extension in the future. But with datatype's "flags" argument and
> enum datatype_flags there are no advantages of this approach.
> 
> - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This could even
>   be a bool:1 bitfield if we cared to represent the information with
>   one bit only. For now it's not done because that would not help reducing
>   the size of the struct, so a bitfield is less preferable.
> 
> - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent static
>   instances. Drop this redundant flag.

Not sure I want to rely on refcnt to zero to identify dynamic
datatypes. I think we need to consolidate datatype_set() to be used
not only where this deals with dynamic datatypes, it might help
improve traceability of datatype assignment.

> - move the integer field "refcnt" in struct datatype beside other fields
>   of similar size/alignment. This makes the size of the struct by one
>   pointer size smaller (on x86-64).
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/datatype.h        | 24 +++++++++---------------
>  src/datatype.c            | 20 ++++++++------------
>  src/meta.c                |  2 +-
>  src/netlink_delinearize.c |  2 +-
>  src/rt.c                  |  2 +-
>  src/segtree.c             |  5 ++---
>  6 files changed, 22 insertions(+), 33 deletions(-)
> 
> diff --git a/include/datatype.h b/include/datatype.h
> index 52a2e943b252..5b85adc15857 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -120,24 +120,13 @@ enum byteorder {
>  
>  struct expr;
>  
> -/**
> - * enum datatype_flags
> - *
> - * @DTYPE_F_ALLOC:		datatype is dynamically allocated
> - * @DTYPE_F_PREFIX:		preferred representation for ranges is a prefix
> - */
> -enum datatype_flags {
> -	DTYPE_F_ALLOC		= (1 << 0),
> -	DTYPE_F_PREFIX		= (1 << 1),
> -};
> -
>  struct parse_ctx;
>  /**
>   * struct datatype
>   *
>   * @type:	numeric identifier
>   * @byteorder:	byteorder of type (non-basetypes only)
> - * @flags:	flags
> + * @f_prefix:	preferred representation for ranges is a prefix
>   * @size:	type size (fixed sized non-basetypes only)
>   * @subtypes:	number of subtypes (concat type)
>   * @name:	type name
> @@ -147,14 +136,20 @@ struct parse_ctx;
>   * @print:	function to print a constant of this type
>   * @parse:	function to parse a symbol and return an expression
>   * @sym_tbl:	symbol table for this type
> - * @refcnt:	reference counter (only for DTYPE_F_ALLOC)
> + * @refcnt:	reference counter for dynamically allocated instances.
>   */
>  struct datatype {
>  	uint32_t			type;
>  	enum byteorder			byteorder;
> -	unsigned int			flags;
> +	bool				f_prefix;
>  	unsigned int			size;
>  	unsigned int			subtypes;
> +
> +	/* Refcount for dynamically allocated instances. For static instances
> +	 * this is zero (get() and free() are NOPs).
> +	 */
> +	unsigned int			refcnt;
> +
>  	const char			*name;
>  	const char			*desc;
>  	const struct datatype		*basetype;
> @@ -169,7 +164,6 @@ struct datatype {
>  	struct error_record		*(*err)(const struct expr *sym);
>  	void				(*describe)(struct output_ctx *octx);
>  	const struct symbol_table	*sym_tbl;
> -	unsigned int			refcnt;
>  };
>  
>  extern const struct datatype *datatype_lookup(enum datatypes type);
> diff --git a/src/datatype.c b/src/datatype.c
> index 70c84846f70e..c5d88d9a90b6 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -641,7 +641,7 @@ const struct datatype ipaddr_type = {
>  	.basetype	= &integer_type,
>  	.print		= ipaddr_type_print,
>  	.parse		= ipaddr_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  static void ip6addr_type_print(const struct expr *expr, struct output_ctx *octx)
> @@ -708,7 +708,7 @@ const struct datatype ip6addr_type = {
>  	.basetype	= &integer_type,
>  	.print		= ip6addr_type_print,
>  	.parse		= ip6addr_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  static void inet_protocol_type_print(const struct expr *expr,
> @@ -944,7 +944,7 @@ const struct datatype mark_type = {
>  	.print		= mark_type_print,
>  	.json		= mark_type_json,
>  	.parse		= mark_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  static const struct symbol_table icmp_code_tbl = {
> @@ -1203,9 +1203,7 @@ static struct datatype *datatype_alloc(void)
>  	struct datatype *dtype;
>  
>  	dtype = xzalloc(sizeof(*dtype));
> -	dtype->flags = DTYPE_F_ALLOC;
>  	dtype->refcnt = 1;
> -
>  	return dtype;
>  }
>  
> @@ -1215,10 +1213,10 @@ struct datatype *datatype_get(const struct datatype *ptr)
>  
>  	if (!dtype)
>  		return NULL;
> -	if (!(dtype->flags & DTYPE_F_ALLOC))
> -		return dtype;
>  
> -	dtype->refcnt++;
> +	if (dtype->refcnt > 0)
> +		dtype->refcnt++;
> +
>  	return dtype;
>  }
>  
> @@ -1245,7 +1243,6 @@ struct datatype *datatype_clone(const struct datatype *orig_dtype)
>  	*dtype = *orig_dtype;
>  	dtype->name = xstrdup(orig_dtype->name);
>  	dtype->desc = xstrdup(orig_dtype->desc);
> -	dtype->flags = DTYPE_F_ALLOC | orig_dtype->flags;
>  	dtype->refcnt = 1;
>  
>  	return dtype;
> @@ -1257,10 +1254,9 @@ void datatype_free(const struct datatype *ptr)
>  
>  	if (!dtype)
>  		return;
> -	if (!(dtype->flags & DTYPE_F_ALLOC))
> -		return;
>  
> -	assert(dtype->refcnt != 0);
> +	if (dtype->refcnt == 0)
> +		return;
>  
>  	if (--dtype->refcnt > 0)
>  		return;
> diff --git a/src/meta.c b/src/meta.c
> index 181e111cbbdc..7bf749b34fb4 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -368,7 +368,7 @@ const struct datatype devgroup_type = {
>  	.print		= devgroup_type_print,
>  	.json		= devgroup_type_json,
>  	.parse		= devgroup_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  const struct datatype ifname_type = {
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index 41cb37a3ccb3..f3939be2d063 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -2568,7 +2568,7 @@ static void relational_binop_postprocess(struct rule_pp_ctx *ctx,
>  		default:
>  			break;
>  		}
> -	} else if (binop->left->dtype->flags & DTYPE_F_PREFIX &&
> +	} else if (binop->left->dtype->f_prefix &&
>  		   binop->op == OP_AND && expr->right->etype == EXPR_VALUE &&
>  		   expr_mask_is_prefix(binop->right)) {
>  		expr->left = expr_get(binop->left);
> diff --git a/src/rt.c b/src/rt.c
> index 9ddcb210eaad..ccea0aa9bc44 100644
> --- a/src/rt.c
> +++ b/src/rt.c
> @@ -55,7 +55,7 @@ const struct datatype realm_type = {
>  	.basetype	= &integer_type,
>  	.print		= realm_type_print,
>  	.parse		= realm_type_parse,
> -	.flags		= DTYPE_F_PREFIX,
> +	.f_prefix	= true,
>  };
>  
>  const struct rt_template rt_templates[] = {
> diff --git a/src/segtree.c b/src/segtree.c
> index 0a12a0cd5151..637457b087b9 100644
> --- a/src/segtree.c
> +++ b/src/segtree.c
> @@ -402,8 +402,7 @@ void concat_range_aggregate(struct expr *set)
>  				goto next;
>  			}
>  
> -			if (prefix_len < 0 ||
> -			    !(r1->dtype->flags & DTYPE_F_PREFIX)) {
> +			if (prefix_len < 0 || !r1->dtype->f_prefix) {
>  				tmp = range_expr_alloc(&r1->location, r1,
>  						       r2);
>  
> @@ -518,7 +517,7 @@ add_interval(struct expr *set, struct expr *low, struct expr *i)
>  		expr = expr_get(low);
>  	} else if (range_is_prefix(range) && !mpz_cmp_ui(p, 0)) {
>  
> -		if (i->dtype->flags & DTYPE_F_PREFIX)
> +		if (i->dtype->f_prefix)
>  			expr = interval_to_prefix(low, i, range);
>  		else if (expr_basetype(i)->type == TYPE_STRING)
>  			expr = interval_to_string(low, i, range);
> -- 
> 2.41.0
> 

Re: [PATCH nft 3/9] datatype: drop flags field from datatype

[Thomas Haller]

On Wed, 2023-09-20 at 20:10 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> > Flags are not always bad. For example, as a function argument they
> > allow
> > easier extension in the future. But with datatype's "flags"
> > argument and
> > enum datatype_flags there are no advantages of this approach.
> > 
> > - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This could
> > even
> >   be a bool:1 bitfield if we cared to represent the information
> > with
> >   one bit only. For now it's not done because that would not help
> > reducing
> >   the size of the struct, so a bitfield is less preferable.
> > 
> > - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent
> > static
> >   instances. Drop this redundant flag.
> 
> Not sure I want to rely on refcnt to zero to identify dynamic
> datatypes. I think we need to consolidate datatype_set() to be used
> not only where this deals with dynamic datatypes, it might help
> improve traceability of datatype assignment.

I don't understand. Could you elaborate about datatype_set()?

Btw, for dynamically allocated instances the refcnt is always positive,
and for static ones it's always zero. The DTYPE_F_ALLOC flag is
redundant.


Thomas

Re: [PATCH nft 3/9] datatype: drop flags field from datatype

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 09:23:46PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-20 at 20:10 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> > > Flags are not always bad. For example, as a function argument they
> > > allow
> > > easier extension in the future. But with datatype's "flags"
> > > argument and
> > > enum datatype_flags there are no advantages of this approach.
> > > 
> > > - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This could
> > > even
> > >   be a bool:1 bitfield if we cared to represent the information
> > > with
> > >   one bit only. For now it's not done because that would not help
> > > reducing
> > >   the size of the struct, so a bitfield is less preferable.
> > > 
> > > - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent
> > > static
> > >   instances. Drop this redundant flag.
> > 
> > Not sure I want to rely on refcnt to zero to identify dynamic
> > datatypes. I think we need to consolidate datatype_set() to be used
> > not only where this deals with dynamic datatypes, it might help
> > improve traceability of datatype assignment.
> 
> I don't understand. Could you elaborate about datatype_set()?

I wonder if we could use datatype_set() to attach static datatypes
too, instead of manually attaching datatypes, such as:

        expr->dtype = &integer_type;

in case of future extensions, using consistently this helper function
will help to identify datatype attachments.

> Btw, for dynamically allocated instances the refcnt is always positive,
> and for static ones it's always zero. The DTYPE_F_ALLOC flag is
> redundant.

That is a correct observation, but a (hipothetical) subtle bug in
refcnt might lead to a dynamic datatype get to refcnt to zero, and
that might be harder to track?

Let me have a look if I can come up with some counter proposal to get
rid of this flag, I would prefer not to infer the datatype class from
reference counter value.

Re: [PATCH nft 3/9] datatype: drop flags field from datatype

[Thomas Haller]

On Thu, 2023-09-21 at 16:23 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 20, 2023 at 09:23:46PM +0200, Thomas Haller wrote:
> > On Wed, 2023-09-20 at 20:10 +0200, Pablo Neira Ayuso wrote:
> > > On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> > > > Flags are not always bad. For example, as a function argument
> > > > they
> > > > allow
> > > > easier extension in the future. But with datatype's "flags"
> > > > argument and
> > > > enum datatype_flags there are no advantages of this approach.
> > > > 
> > > > - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This
> > > > could
> > > > even
> > > >   be a bool:1 bitfield if we cared to represent the information
> > > > with
> > > >   one bit only. For now it's not done because that would not
> > > > help
> > > > reducing
> > > >   the size of the struct, so a bitfield is less preferable.
> > > > 
> > > > - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent
> > > > static
> > > >   instances. Drop this redundant flag.
> > > 
> > > Not sure I want to rely on refcnt to zero to identify dynamic
> > > datatypes. I think we need to consolidate datatype_set() to be
> > > used
> > > not only where this deals with dynamic datatypes, it might help
> > > improve traceability of datatype assignment.
> > 
> > I don't understand. Could you elaborate about datatype_set()?
> 
> I wonder if we could use datatype_set() to attach static datatypes
> too, instead of manually attaching datatypes, such as:
> 
>         expr->dtype = &integer_type;
> 
> in case of future extensions, using consistently this helper function
> will help to identify datatype attachments.

I think `expr->dtype = &integer_type` is fine, if

- expr->dtype doesn't previously point to a datatype that requires
datatype_free() (e.g. because it's NULL).

- the new datatype requires no datatype_get() (e.g. because it's
static).

> 
> > Btw, for dynamically allocated instances the refcnt is always
> > positive,
> > and for static ones it's always zero. The DTYPE_F_ALLOC flag is
> > redundant.
> 
> That is a correct observation, but a (hipothetical) subtle bug in
> refcnt might lead to a dynamic datatype get to refcnt to zero, and
> that might be harder to track?
> 
> Let me have a look if I can come up with some counter proposal to get
> rid of this flag, I would prefer not to infer the datatype class from
> reference counter value.

If the reference counting is messed up, there is either a leak, a use-
after-free or modification of static data. These are all bad bugs that
needs fixing and are avoided by best practices and testing.

IMO keeping redundant state does not help with that or with
readability.



I'd like to replace the "unsigned int flags" field with individual
boolean fields like "bool f_prefix" or "bool f_alloc".

Dropping DTYPE_F_ALLOC/f_alloc flag altogether can be done (or not
done) independently from that.


Thomas

Re: [PATCH nft 3/9] datatype: drop flags field from datatype

[Pablo Neira Ayuso]

On Fri, Sep 22, 2023 at 10:51:29AM +0200, Thomas Haller wrote:
> On Thu, 2023-09-21 at 16:23 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 20, 2023 at 09:23:46PM +0200, Thomas Haller wrote:
> > > On Wed, 2023-09-20 at 20:10 +0200, Pablo Neira Ayuso wrote:
> > > > On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> > > > > Flags are not always bad. For example, as a function argument
> > > > > they
> > > > > allow
> > > > > easier extension in the future. But with datatype's "flags"
> > > > > argument and
> > > > > enum datatype_flags there are no advantages of this approach.
> > > > > 
> > > > > - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This
> > > > > could
> > > > > even
> > > > >   be a bool:1 bitfield if we cared to represent the information
> > > > > with
> > > > >   one bit only. For now it's not done because that would not
> > > > > help
> > > > > reducing
> > > > >   the size of the struct, so a bitfield is less preferable.
> > > > > 
> > > > > - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent
> > > > > static
> > > > >   instances. Drop this redundant flag.
> > > > 
> > > > Not sure I want to rely on refcnt to zero to identify dynamic
> > > > datatypes. I think we need to consolidate datatype_set() to be
> > > > used
> > > > not only where this deals with dynamic datatypes, it might help
> > > > improve traceability of datatype assignment.
> > > 
> > > I don't understand. Could you elaborate about datatype_set()?
> > 
> > I wonder if we could use datatype_set() to attach static datatypes
> > too, instead of manually attaching datatypes, such as:
> > 
> >         expr->dtype = &integer_type;
> > 
> > in case of future extensions, using consistently this helper function
> > will help to identify datatype attachments.
> 
> I think `expr->dtype = &integer_type` is fine, if
> 
> - expr->dtype doesn't previously point to a datatype that requires
> datatype_free() (e.g. because it's NULL).
> 
> - the new datatype requires no datatype_get() (e.g. because it's
> static).
> 
> > 
> > > Btw, for dynamically allocated instances the refcnt is always
> > > positive,
> > > and for static ones it's always zero. The DTYPE_F_ALLOC flag is
> > > redundant.
> > 
> > That is a correct observation, but a (hipothetical) subtle bug in
> > refcnt might lead to a dynamic datatype get to refcnt to zero, and
> > that might be harder to track?
> > 
> > Let me have a look if I can come up with some counter proposal to get
> > rid of this flag, I would prefer not to infer the datatype class from
> > reference counter value.
> 
> If the reference counting is messed up, there is either a leak, a use-
> after-free or modification of static data.

I am refering to a hypothetical dynamic datatype object reaching
refcnt == 0.

> These are all bad bugs that needs fixing and are avoided by best
> practices and testing.
> 
> IMO keeping redundant state does not help with that or with
> readability.
> 
> I'd like to replace the "unsigned int flags" field with individual
> boolean fields like "bool f_prefix" or "bool f_alloc".

As for f_prefix, there is probably a way to infer this from context,
I would need to have a look.

> Dropping DTYPE_F_ALLOC/f_alloc flag altogether can be done (or not
> done) independently from that.

[PATCH nft 4/9] datatype: use "enum byteorder" instead of int in set_datatype_alloc()

[Thomas Haller]

Use the enum types as we have them.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/datatype.h | 2 +-
 src/datatype.c     | 2 +-
 src/evaluate.c     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 5b85adc15857..202935bd322f 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -295,7 +295,7 @@ concat_subtype_lookup(uint32_t type, unsigned int n)
 }
 
 extern const struct datatype *
-set_datatype_alloc(const struct datatype *orig_dtype, unsigned int byteorder);
+set_datatype_alloc(const struct datatype *orig_dtype, enum byteorder byteorder);
 
 extern void time_print(uint64_t msec, struct output_ctx *octx);
 extern struct error_record *time_parse(const struct location *loc,
diff --git a/src/datatype.c b/src/datatype.c
index c5d88d9a90b6..6e4bfc4c0de7 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1304,7 +1304,7 @@ const struct datatype *concat_type_alloc(uint32_t type)
 }
 
 const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
-					  unsigned int byteorder)
+					  enum byteorder byteorder)
 {
 	struct datatype *dtype;
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 03586922848a..933fddd8996d 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1341,7 +1341,7 @@ static int expr_evaluate_bitwise(struct eval_ctx *ctx, struct expr **expr)
 	struct expr *op = *expr, *left = op->left;
 	const struct datatype *dtype;
 	unsigned int max_len;
-	int byteorder;
+	enum byteorder byteorder;
 
 	if (ctx->stmt_len > left->len) {
 		max_len = ctx->stmt_len;
-- 
2.41.0

Re: [PATCH nft 4/9] datatype: use "enum byteorder" instead of int in set_datatype_alloc()

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:05PM +0200, Thomas Haller wrote:
> Use the enum types as we have them.

Applied, thanks

[PATCH nft 5/9] payload: use enum icmp_hdr_field_type in payload_may_dependency_kill_icmp()

[Thomas Haller]

Don't mix icmp_dep (enum icmp_hdr_field_type) and the uint8_t icmp_type.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/payload.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/payload.c b/src/payload.c
index a02942b3382a..cb8edfac0338 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -802,18 +802,16 @@ static uint8_t icmp_dep_to_type(enum icmp_hdr_field_type t)
 static bool payload_may_dependency_kill_icmp(struct payload_dep_ctx *ctx, struct expr *expr)
 {
 	const struct expr *dep = payload_dependency_get(ctx, expr->payload.base);
-	uint8_t icmp_type;
+	enum icmp_hdr_field_type icmp_dep;
 
-	icmp_type = expr->payload.tmpl->icmp_dep;
-	if (icmp_type == PROTO_ICMP_ANY)
+	icmp_dep = expr->payload.tmpl->icmp_dep;
+	if (icmp_dep == PROTO_ICMP_ANY)
 		return false;
 
 	if (dep->left->payload.desc != expr->payload.desc)
 		return false;
 
-	icmp_type = icmp_dep_to_type(expr->payload.tmpl->icmp_dep);
-
-	return ctx->icmp_type == icmp_type;
+	return ctx->icmp_type == icmp_dep_to_type(icmp_dep);
 }
 
 static bool payload_may_dependency_kill_ll(struct payload_dep_ctx *ctx, struct expr *expr)
-- 
2.41.0

Re: [PATCH nft 5/9] payload: use enum icmp_hdr_field_type in payload_may_dependency_kill_icmp()

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:06PM +0200, Thomas Haller wrote:
> Don't mix icmp_dep (enum icmp_hdr_field_type) and the uint8_t icmp_type.

Applied, thanks

[PATCH nft 6/9] netlink: handle invalid etype in set_make_key()

[Thomas Haller]

It's not clear to me, what ensures that the etype is always valid.
Handle a NULL.

Fixes: 6e48df5329ea ('src: add "typeof" build/parse/print support')

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/netlink.c b/src/netlink.c
index 2489e9864151..70ebf382b14f 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -896,6 +896,8 @@ static struct expr *set_make_key(const struct nftnl_udata *attr)
 
 	etype = nftnl_udata_get_u32(ud[NFTNL_UDATA_SET_TYPEOF_EXPR]);
 	ops = expr_ops_by_type(etype);
+	if (!ops)
+		return NULL;
 
 	expr = ops->parse_udata(ud[NFTNL_UDATA_SET_TYPEOF_DATA]);
 	if (!expr)
-- 
2.41.0

Re: [PATCH nft 6/9] netlink: handle invalid etype in set_make_key()

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:07PM +0200, Thomas Haller wrote:
> It's not clear to me, what ensures that the etype is always valid.
> Handle a NULL.

Applied, thanks

Re: [PATCH nft 6/9] netlink: handle invalid etype in set_make_key()

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:07PM +0200, Thomas Haller wrote:
> It's not clear to me, what ensures that the etype is always valid.
> Handle a NULL.

Applied, thanks.

> Fixes: 6e48df5329ea ('src: add "typeof" build/parse/print support')

Please remove empty line break between Fixes: tag and Signed-off-by:
in your future patches.

Thanks!

[PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Thomas Haller]

Be more careful about casting an uint32_t value to "enum expr_types" and
make fewer assumptions about the underlying integer type of the enum.
Instead, be clear about where we have an untrusted uint32_t from netlink
and an enum. Rename expr_ops_by_type() to expr_ops_by_type_u32() to make
this clearer. Later we might make the enum as packed, when this starts
to matter more.

Also, only the code path expr_ops() wants strict validation and assert
against valid enum values. Move the assertion out of
__expr_ops_by_type(). Then expr_ops_by_type_u32() does not need to
duplicate the handling of EXPR_INVALID. We still need to duplicate the
check against EXPR_MAX, to ensure that the uint32_t value can be cast to
an enum value.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/expression.h |  2 +-
 src/expression.c     | 23 +++++++++++------------
 src/netlink.c        |  4 ++--
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index 469f41ecd613..aede223db741 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -189,7 +189,7 @@ struct expr_ops {
 };
 
 const struct expr_ops *expr_ops(const struct expr *e);
-const struct expr_ops *expr_ops_by_type(enum expr_types etype);
+const struct expr_ops *expr_ops_by_type_u32(uint32_t value);
 
 /**
  * enum expr_flags
diff --git a/src/expression.c b/src/expression.c
index 87d5a9fcbe09..320c02be522c 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -995,7 +995,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
 			goto err_free;
 
 		etype = nftnl_udata_get_u32(nest_ud[NFTNL_UDATA_SET_KEY_CONCAT_SUB_TYPE]);
-		ops = expr_ops_by_type(etype);
+		ops = expr_ops_by_type_u32(etype);
 		if (!ops || !ops->parse_udata)
 			goto err_free;
 
@@ -1509,9 +1509,7 @@ void range_expr_value_high(mpz_t rop, const struct expr *expr)
 static const struct expr_ops *__expr_ops_by_type(enum expr_types etype)
 {
 	switch (etype) {
-	case EXPR_INVALID:
-		BUG("Invalid expression ops requested");
-		break;
+	case EXPR_INVALID: break;
 	case EXPR_VERDICT: return &verdict_expr_ops;
 	case EXPR_SYMBOL: return &symbol_expr_ops;
 	case EXPR_VARIABLE: return &variable_expr_ops;
@@ -1543,21 +1541,22 @@ static const struct expr_ops *__expr_ops_by_type(enum expr_types etype)
 	case EXPR_FLAGCMP: return &flagcmp_expr_ops;
 	}
 
-	BUG("Unknown expression type %d\n", etype);
+	return NULL;
 }
 
 const struct expr_ops *expr_ops(const struct expr *e)
 {
-	return __expr_ops_by_type(e->etype);
+	const struct expr_ops *ops;
+
+	ops = __expr_ops_by_type(e->etype);
+	if (!ops)
+		BUG("Unknown expression type %d\n", e->etype);
+	return ops;
 }
 
-const struct expr_ops *expr_ops_by_type(enum expr_types value)
+const struct expr_ops *expr_ops_by_type_u32(uint32_t value)
 {
-	/* value might come from unreliable source, such as "udata"
-	 * annotation of set keys.  Avoid BUG() assertion.
-	 */
-	if (value == EXPR_INVALID || value > EXPR_MAX)
+	if (value > (uint32_t) EXPR_MAX)
 		return NULL;
-
 	return __expr_ops_by_type(value);
 }
diff --git a/src/netlink.c b/src/netlink.c
index 70ebf382b14f..8af579c7b778 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -878,8 +878,8 @@ static struct expr *set_make_key(const struct nftnl_udata *attr)
 {
 	const struct nftnl_udata *ud[NFTNL_UDATA_SET_TYPEOF_MAX + 1] = {};
 	const struct expr_ops *ops;
-	enum expr_types etype;
 	struct expr *expr;
+	uint32_t etype;
 	int err;
 
 	if (!attr)
@@ -895,7 +895,7 @@ static struct expr *set_make_key(const struct nftnl_udata *attr)
 		return NULL;
 
 	etype = nftnl_udata_get_u32(ud[NFTNL_UDATA_SET_TYPEOF_EXPR]);
-	ops = expr_ops_by_type(etype);
+	ops = expr_ops_by_type_u32(etype);
 	if (!ops)
 		return NULL;
 
-- 
2.41.0

Re: [PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:08PM +0200, Thomas Haller wrote:
> Be more careful about casting an uint32_t value to "enum expr_types" and
> make fewer assumptions about the underlying integer type of the enum.
> Instead, be clear about where we have an untrusted uint32_t from netlink
> and an enum. Rename expr_ops_by_type() to expr_ops_by_type_u32() to make
> this clearer. Later we might make the enum as packed, when this starts
> to matter more.
> 
> Also, only the code path expr_ops() wants strict validation and assert
> against valid enum values. Move the assertion out of
> __expr_ops_by_type(). Then expr_ops_by_type_u32() does not need to
> duplicate the handling of EXPR_INVALID. We still need to duplicate the
> check against EXPR_MAX, to ensure that the uint32_t value can be cast to
> an enum value.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/expression.h |  2 +-
>  src/expression.c     | 23 +++++++++++------------
>  src/netlink.c        |  4 ++--
>  3 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/include/expression.h b/include/expression.h
> index 469f41ecd613..aede223db741 100644
> --- a/include/expression.h
> +++ b/include/expression.h
> @@ -189,7 +189,7 @@ struct expr_ops {
>  };
>  
>  const struct expr_ops *expr_ops(const struct expr *e);
> -const struct expr_ops *expr_ops_by_type(enum expr_types etype);
> +const struct expr_ops *expr_ops_by_type_u32(uint32_t value);
>  
>  /**
>   * enum expr_flags
> diff --git a/src/expression.c b/src/expression.c
> index 87d5a9fcbe09..320c02be522c 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -995,7 +995,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
>  			goto err_free;
>  
>  		etype = nftnl_udata_get_u32(nest_ud[NFTNL_UDATA_SET_KEY_CONCAT_SUB_TYPE]);
> -		ops = expr_ops_by_type(etype);
> +		ops = expr_ops_by_type_u32(etype);
>  		if (!ops || !ops->parse_udata)
>  			goto err_free;
>  
> @@ -1509,9 +1509,7 @@ void range_expr_value_high(mpz_t rop, const struct expr *expr)
>  static const struct expr_ops *__expr_ops_by_type(enum expr_types etype)
>  {
>  	switch (etype) {
> -	case EXPR_INVALID:
> -		BUG("Invalid expression ops requested");
> -		break;
> +	case EXPR_INVALID: break;
>  	case EXPR_VERDICT: return &verdict_expr_ops;
>  	case EXPR_SYMBOL: return &symbol_expr_ops;
>  	case EXPR_VARIABLE: return &variable_expr_ops;
> @@ -1543,21 +1541,22 @@ static const struct expr_ops *__expr_ops_by_type(enum expr_types etype)
>  	case EXPR_FLAGCMP: return &flagcmp_expr_ops;
>  	}
>  
> -	BUG("Unknown expression type %d\n", etype);
> +	return NULL;
>  }
>  
>  const struct expr_ops *expr_ops(const struct expr *e)
>  {
> -	return __expr_ops_by_type(e->etype);
> +	const struct expr_ops *ops;
> +
> +	ops = __expr_ops_by_type(e->etype);
> +	if (!ops)
> +		BUG("Unknown expression type %d\n", e->etype);
> +	return ops;
>  }
>  
> -const struct expr_ops *expr_ops_by_type(enum expr_types value)
> +const struct expr_ops *expr_ops_by_type_u32(uint32_t value)
>  {
> -	/* value might come from unreliable source, such as "udata"
> -	 * annotation of set keys.  Avoid BUG() assertion.
> -	 */
> -	if (value == EXPR_INVALID || value > EXPR_MAX)
> +	if (value > (uint32_t) EXPR_MAX)

I think this still allows a third party to set EXPR_INVALID in the
netlink userdata attribute, right?

>  		return NULL;
> -
>  	return __expr_ops_by_type(value);
>  }
> diff --git a/src/netlink.c b/src/netlink.c
> index 70ebf382b14f..8af579c7b778 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -878,8 +878,8 @@ static struct expr *set_make_key(const struct nftnl_udata *attr)
>  {
>  	const struct nftnl_udata *ud[NFTNL_UDATA_SET_TYPEOF_MAX + 1] = {};
>  	const struct expr_ops *ops;
> -	enum expr_types etype;
>  	struct expr *expr;
> +	uint32_t etype;
>  	int err;
>  
>  	if (!attr)
> @@ -895,7 +895,7 @@ static struct expr *set_make_key(const struct nftnl_udata *attr)
>  		return NULL;
>  
>  	etype = nftnl_udata_get_u32(ud[NFTNL_UDATA_SET_TYPEOF_EXPR]);
> -	ops = expr_ops_by_type(etype);
> +	ops = expr_ops_by_type_u32(etype);
>  	if (!ops)
>  		return NULL;
>  
> -- 
> 2.41.0
> 

Re: [PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Thomas Haller]

On Wed, 2023-09-20 at 20:13 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 20, 2023 at 04:26:08PM +0200, Thomas Haller wrote:
> > 
> > -const struct expr_ops *expr_ops_by_type(enum expr_types value)
> > +const struct expr_ops *expr_ops_by_type_u32(uint32_t value)
> >  {
> > -       /* value might come from unreliable source, such as "udata"
> > -        * annotation of set keys.  Avoid BUG() assertion.
> > -        */
> > -       if (value == EXPR_INVALID || value > EXPR_MAX)
> > +       if (value > (uint32_t) EXPR_MAX)
> 
> I think this still allows a third party to set EXPR_INVALID in the
> netlink userdata attribute, right?
> 
> >                 return NULL;
> > -
> >         return __expr_ops_by_type(value);
> >  }

Yes, it still allows that. It's handled by the following
__expr_ops_by_type(), which returns NULL for invalid types (like
EXPR_INVALID).

The check "if (value > (uint32_t) EXPR_MAX)" is only here to ensure
that nothing is lost while casting the uint32_t "value" to the enum
expr_types.


Thomas

Re: [PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 09:28:29PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-20 at 20:13 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 20, 2023 at 04:26:08PM +0200, Thomas Haller wrote:
> > > 
> > > -const struct expr_ops *expr_ops_by_type(enum expr_types value)
> > > +const struct expr_ops *expr_ops_by_type_u32(uint32_t value)
> > >  {
> > > -       /* value might come from unreliable source, such as "udata"
> > > -        * annotation of set keys.  Avoid BUG() assertion.
> > > -        */
> > > -       if (value == EXPR_INVALID || value > EXPR_MAX)
> > > +       if (value > (uint32_t) EXPR_MAX)
> > 
> > I think this still allows a third party to set EXPR_INVALID in the
> > netlink userdata attribute, right?
> > 
> > >                 return NULL;
> > > -
> > >         return __expr_ops_by_type(value);
> > >  }
> 
> Yes, it still allows that. It's handled by the following
> __expr_ops_by_type(), which returns NULL for invalid types (like
> EXPR_INVALID).

Oh indeed.

> The check "if (value > (uint32_t) EXPR_MAX)" is only here to ensure
> that nothing is lost while casting the uint32_t "value" to the enum
> expr_types.

Is this cast really required? This is to handle the hypothetical case
where EXPR_MAX ever gets a negative value?

Re: [PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Thomas Haller]

On Thu, 2023-09-21 at 16:19 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 20, 2023 at 09:28:29PM +0200, Thomas Haller wrote:
> 
> 
> > The check "if (value > (uint32_t) EXPR_MAX)" is only here to ensure
> > that nothing is lost while casting the uint32_t "value" to the enum
> > expr_types.
> 
> Is this cast really required? This is to handle the hypothetical case
> where EXPR_MAX ever gets a negative value?
> 

EXPR_MAX is never negative.

If EXPR_MAX is treated as a signed integer, then it will be implicitly
cast to unsigned when comparing with the uint32_t. The behavior will be
correct without a cast.

But won't the compiler warn about comparing integers of different
signedness?

The cast is probably not needed. But it doesn't hurt.


Thomas

Re: [PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Pablo Neira Ayuso]

On Fri, Sep 22, 2023 at 10:54:52AM +0200, Thomas Haller wrote:
> On Thu, 2023-09-21 at 16:19 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 20, 2023 at 09:28:29PM +0200, Thomas Haller wrote:
> > 
> > 
> > > The check "if (value > (uint32_t) EXPR_MAX)" is only here to ensure
> > > that nothing is lost while casting the uint32_t "value" to the enum
> > > expr_types.
> > 
> > Is this cast really required? This is to handle the hypothetical case
> > where EXPR_MAX ever gets a negative value?
> > 
> 
> EXPR_MAX is never negative.
> 
> If EXPR_MAX is treated as a signed integer, then it will be implicitly
> cast to unsigned when comparing with the uint32_t. The behavior will be
> correct without a cast.
> 
> But won't the compiler warn about comparing integers of different
> signedness?
> 
> The cast is probably not needed. But it doesn't hurt.

I'd prefer to remove, if not strictly necessary. EXPR_* are never
expected to see a negative value.

Re: [PATCH nft 7/9] expression: cleanup expr_ops_by_type() and handle u32 input

[Pablo Neira Ayuso]

On Fri, Sep 22, 2023 at 10:54:52AM +0200, Thomas Haller wrote:
> On Thu, 2023-09-21 at 16:19 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 20, 2023 at 09:28:29PM +0200, Thomas Haller wrote:
> > 
> > 
> > > The check "if (value > (uint32_t) EXPR_MAX)" is only here to ensure
> > > that nothing is lost while casting the uint32_t "value" to the enum
> > > expr_types.
> > 
> > Is this cast really required? This is to handle the hypothetical case
> > where EXPR_MAX ever gets a negative value?
> > 
> 
> EXPR_MAX is never negative.
> 
> If EXPR_MAX is treated as a signed integer, then it will be implicitly
> cast to unsigned when comparing with the uint32_t. The behavior will be
> correct without a cast.
> 
> But won't the compiler warn about comparing integers of different
> signedness?
> 
> The cast is probably not needed. But it doesn't hurt.

Applied, I removed the cast and I added a note in the commit that I
mangled this myself, it is really turns out we really need it, we can
recover it. Thanks.

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

[Thomas Haller]

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))
 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

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

[Pablo Neira Ayuso]

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.

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

[Thomas Haller]

On Wed, 2023-09-20 at 18:02 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 20, 2023 at 04:26:09PM +0200, Thomas Haller wrote:
> 
> > +       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.

The packed enums are only one uint8_t large. The memory-saving compared
to a :8 bitfield is exactly the same.

Thomas

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

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 07:48:22PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-20 at 18:02 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 20, 2023 at 04:26:09PM +0200, Thomas Haller wrote:
> > 
> > > +       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.
> 
> The packed enums are only one uint8_t large. The memory-saving compared
> to a :8 bitfield is exactly the same.

I see, I am not sure yet I want to give control to the compiler on the
layout of these structures, I have been using pahole to shrink them
further, specifically struct expr is critical (I have at least one
patch to reduce the size of constant expressions to safe % memory with
very 13.5%. And location for error reporting is still consuming 48
bytes per struct expr.

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

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:09PM +0200, Thomas Haller wrote:
> diff --git a/include/proto.h b/include/proto.h

Not related, but this reminds me, there are duplicated protocol header
definitions in include/proto.h that can possibly go away by using libc
definitions.

[PATCH nft 9/9] proto: add missing proto_definitions for PROTO_DESC_GENEVE

[Thomas Haller]

While at it, make proto_definitions const. For global variables, this
allows the linker to mark the memory as read only. It's just good to do
by default.

Fixes: 156d22654003 ('src: add geneve matching support')

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/proto.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/proto.c b/src/proto.c
index b5cb0106dd7b..735e37f850c5 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -1281,7 +1281,7 @@ const struct proto_desc proto_netdev = {
 	},
 };
 
-static const struct proto_desc *proto_definitions[PROTO_DESC_MAX + 1] = {
+static const struct proto_desc *const proto_definitions[PROTO_DESC_MAX + 1] = {
 	[PROTO_DESC_AH]		= &proto_ah,
 	[PROTO_DESC_ESP]	= &proto_esp,
 	[PROTO_DESC_COMP]	= &proto_comp,
@@ -1300,6 +1300,7 @@ static const struct proto_desc *proto_definitions[PROTO_DESC_MAX + 1] = {
 	[PROTO_DESC_VLAN]	= &proto_vlan,
 	[PROTO_DESC_ETHER]	= &proto_eth,
 	[PROTO_DESC_VXLAN]	= &proto_vxlan,
+	[PROTO_DESC_GENEVE]	= &proto_geneve,
 	[PROTO_DESC_GRE]	= &proto_gre,
 	[PROTO_DESC_GRETAP]	= &proto_gretap,
 };
-- 
2.41.0

Re: [PATCH nft 9/9] proto: add missing proto_definitions for PROTO_DESC_GENEVE

[Pablo Neira Ayuso]

On Wed, Sep 20, 2023 at 04:26:10PM +0200, Thomas Haller wrote:
> While at it, make proto_definitions const. For global variables, this
> allows the linker to mark the memory as read only. It's just good to do
> by default.

Applied, thanks