[PATCH nft,v2] src: add reference counter for dynamic datatypes

[PATCH nft,v2] src: add reference counter for dynamic datatypes

[Pablo Neira Ayuso]

Concatenations need a dynamic datatype. Set references use the datatype
from the set. Add a reference counter to deal with releasing the
dynamic datatype object when it is shared, which occurs from the
following paths:

* netlink_delinearize_setelem(), when elements refer to the datatype of
  an existing set in the cache.
* set_expr_alloc(), for implicit set/maps and update statement from the
  packet path.
* set_ref_expr_alloc(), for set and map references via @.
* expr_evaluate_set_elem(), when elements are added and they refer to
  the set datatype, ie. add element command.

==28352== 1,350 (440 direct, 910 indirect) bytes in 5 blocks are definitely lost in loss recor 3 of 3
==28352==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==28352==    by 0x4E79558: xmalloc (utils.c:36)
==28352==    by 0x4E7963D: xzalloc (utils.c:65)
==28352==    by 0x4E6029B: dtype_alloc (datatype.c:1073)
==28352==    by 0x4E6029B: concat_type_alloc (datatype.c:1127)
==28352==    by 0x4E6D3B3: netlink_delinearize_set (netlink.c:578)
==28352==    by 0x4E6D68E: list_set_cb (netlink.c:648)
==28352==    by 0x5D74023: nftnl_set_list_foreach (set.c:780)
==28352==    by 0x4E6D6F3: netlink_list_sets (netlink.c:669)
==28352==    by 0x4E5A7A3: cache_init_objects (rule.c:159)
==28352==    by 0x4E5A7A3: cache_init (rule.c:216)
==28352==    by 0x4E5A7A3: cache_update (rule.c:266)
==28352==    by 0x4E7E0EE: nft_evaluate (libnftables.c:388)
==28352==    by 0x4E7EADD: nft_run_cmd_from_filename (libnftables.c:479)
==28352==    by 0x109A53: main (main.c:310)

Remove DTYPE_F_CLONE flag, which is replaced by proper reference counter.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: deal with expr_evaluate_set_elem() path too.

 include/datatype.h |  6 ++++--
 src/datatype.c     | 38 ++++++++++++++++++++++++++++----------
 src/evaluate.c     |  4 ++--
 src/expression.c   | 11 ++++++-----
 src/netlink.c      |  2 +-
 5 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 14ece282902c..5765c1ba7502 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -117,12 +117,10 @@ struct expr;
  *
  * @DTYPE_F_ALLOC:		datatype is dynamically allocated
  * @DTYPE_F_PREFIX:		preferred representation for ranges is a prefix
- * @DTYPE_F_CLONE:		this is an instance from original datatype
  */
 enum datatype_flags {
 	DTYPE_F_ALLOC		= (1 << 0),
 	DTYPE_F_PREFIX		= (1 << 1),
-	DTYPE_F_CLONE		= (1 << 2),
 };
 
 /**
@@ -140,6 +138,7 @@ enum datatype_flags {
  * @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)
  */
 struct datatype {
 	uint32_t			type;
@@ -158,10 +157,13 @@ struct datatype {
 	struct error_record		*(*parse)(const struct expr *sym,
 						  struct expr **res);
 	const struct symbol_table	*sym_tbl;
+	unsigned int			refcnt;
 };
 
 extern const struct datatype *datatype_lookup(enum datatypes type);
 extern const struct datatype *datatype_lookup_byname(const char *name);
+extern struct datatype *datatype_get(const struct datatype *dtype);
+extern void datatype_free(const struct datatype *dtype);
 
 extern struct error_record *symbol_parse(const struct expr *sym,
 					 struct expr **res);
diff --git a/src/datatype.c b/src/datatype.c
index 1d5ed6f798de..94abc91d8112 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1072,10 +1072,22 @@ static struct datatype *dtype_alloc(void)
 
 	dtype = xzalloc(sizeof(*dtype));
 	dtype->flags = DTYPE_F_ALLOC;
+	dtype->refcnt = 1;
 
 	return dtype;
 }
 
+struct datatype *datatype_get(const struct datatype *ptr)
+{
+	struct datatype *dtype = (struct datatype *)ptr;
+
+	if (!(dtype->flags & DTYPE_F_ALLOC))
+		return dtype;
+
+	dtype->refcnt++;
+	return dtype;
+}
+
 static struct datatype *dtype_clone(const struct datatype *orig_dtype)
 {
 	struct datatype *dtype;
@@ -1084,18 +1096,24 @@ static struct datatype *dtype_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 | DTYPE_F_CLONE;
+	dtype->flags = DTYPE_F_ALLOC;
+	dtype->refcnt = 1;
 
 	return dtype;
 }
 
-static void dtype_free(const struct datatype *dtype)
+void datatype_free(const struct datatype *ptr)
 {
-	if (dtype->flags & DTYPE_F_ALLOC) {
-		xfree(dtype->name);
-		xfree(dtype->desc);
-		xfree(dtype);
-	}
+	struct datatype *dtype = (struct datatype *)ptr;
+
+	if (!(dtype->flags & DTYPE_F_ALLOC))
+		return;
+	if (--dtype->refcnt > 0)
+		return;
+
+	xfree(dtype->name);
+	xfree(dtype->desc);
+	xfree(dtype);
 }
 
 const struct datatype *concat_type_alloc(uint32_t type)
@@ -1137,7 +1155,7 @@ const struct datatype *concat_type_alloc(uint32_t type)
 
 void concat_type_destroy(const struct datatype *dtype)
 {
-	dtype_free(dtype);
+	datatype_free(dtype);
 }
 
 const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
@@ -1157,8 +1175,8 @@ const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
 
 void set_datatype_destroy(const struct datatype *dtype)
 {
-	if (dtype && dtype->flags & DTYPE_F_CLONE)
-		dtype_free(dtype);
+	if (dtype)
+		datatype_free(dtype);
 }
 
 static struct error_record *time_unit_parse(const struct location *loc,
diff --git a/src/evaluate.c b/src/evaluate.c
index 39101b486b2f..6018c96edefe 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1239,7 +1239,7 @@ static int expr_evaluate_set_elem(struct eval_ctx *ctx, struct expr **expr)
 		}
 	}
 
-	elem->dtype = elem->key->dtype;
+	elem->dtype = datatype_get(elem->key->dtype);
 	elem->len   = elem->key->len;
 	elem->flags = elem->key->flags;
 	return 0;
@@ -1285,7 +1285,7 @@ static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
 
 	set->set_flags |= NFT_SET_CONSTANT;
 
-	set->dtype = ctx->ectx.dtype;
+	set->dtype = datatype_get(ctx->ectx.dtype);
 	set->len   = ctx->ectx.len;
 	set->flags |= EXPR_F_CONSTANT;
 	return 0;
diff --git a/src/expression.c b/src/expression.c
index ef694f2a194d..d1cfb24681d9 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -57,8 +57,8 @@ struct expr *expr_clone(const struct expr *expr)
 {
 	struct expr *new;
 
-	new = expr_alloc(&expr->location, expr->etype, expr->dtype,
-			 expr->byteorder, expr->len);
+	new = expr_alloc(&expr->location, expr->etype,
+			 datatype_get(expr->dtype), expr->byteorder, expr->len);
 	new->flags = expr->flags;
 	new->op    = expr->op;
 	expr_ops(expr)->clone(new, expr);
@@ -86,6 +86,8 @@ void expr_free(struct expr *expr)
 	if (--expr->refcnt > 0)
 		return;
 
+	datatype_free(expr->dtype);
+
 	/* EXPR_INVALID expressions lack ->ops structure.
 	 * This happens for compound types.
 	 */
@@ -803,7 +805,6 @@ void compound_expr_remove(struct expr *compound, struct expr *expr)
 
 static void concat_expr_destroy(struct expr *expr)
 {
-	concat_type_destroy(expr->dtype);
 	compound_expr_destroy(expr);
 }
 
@@ -936,7 +937,7 @@ struct expr *set_expr_alloc(const struct location *loc, const struct set *set)
 		return set_expr;
 
 	set_expr->set_flags = set->flags;
-	set_expr->dtype = set->key->dtype;
+	set_expr->dtype = datatype_get(set->key->dtype);
 
 	return set_expr;
 }
@@ -1067,7 +1068,7 @@ struct expr *set_ref_expr_alloc(const struct location *loc, struct set *set)
 {
 	struct expr *expr;
 
-	expr = expr_alloc(loc, EXPR_SET_REF, set->key->dtype, 0, 0);
+	expr = expr_alloc(loc, EXPR_SET_REF, datatype_get(set->key->dtype), 0, 0);
 	expr->set = set_get(set);
 	expr->flags |= EXPR_F_CONSTANT;
 	return expr;
diff --git a/src/netlink.c b/src/netlink.c
index 7a4312498ce8..c5567c0d29ca 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -767,7 +767,7 @@ int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
 		flags = nftnl_set_elem_get_u32(nlse, NFTNL_SET_ELEM_FLAGS);
 
 	key = netlink_alloc_value(&netlink_location, &nld);
-	key->dtype	= set->key->dtype;
+	key->dtype	= datatype_get(set->key->dtype);
 	key->byteorder	= set->key->byteorder;
 	if (set->key->dtype->subtypes)
 		key = netlink_parse_concat_elem(set->key->dtype, key);
-- 
2.11.0