netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH nft 1/3,v3] src: add reference counter for dynamic datatypes
@ 2019-06-12 12:23 Pablo Neira Ayuso
  2019-06-12 12:23 ` [PATCH nft 2/3] datatype: add datatype_set() Pablo Neira Ayuso
  2019-06-12 12:23 ` [PATCH nft 3/3] datatype: dtype_clone() should clone flags too Pablo Neira Ayuso
  0 siblings, 2 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2019-06-12 12:23 UTC (permalink / raw)
  To: netfilter-devel

Two datatypes are using runtime datatype allocation:

* Concatenations.
* Integer, that require byteorder adjustment.

Add a reference counter to release 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.

valgrind reports memleaks like this one:

==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.  Call datatype_free() from expr_free() path.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v3: deal with dynamic integer datatype allocations.

 include/datatype.h |  6 ++++--
 src/datatype.c     | 42 +++++++++++++++++++++++++++++++-----------
 src/evaluate.c     | 24 ++++++++++++------------
 src/expression.c   | 11 ++++++-----
 src/netlink.c      |  2 +-
 src/segtree.c      |  9 +++++----
 6 files changed, 59 insertions(+), 35 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..2aabddbdd0ec 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,26 @@ 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)
+		return;
+	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 +1157,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,
@@ -1152,13 +1172,13 @@ const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
 	dtype = dtype_clone(orig_dtype);
 	dtype->byteorder = byteorder;
 
-	return dtype;
+	return datatype_get(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..904e65718ed5 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -229,7 +229,7 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 
 	switch ((*expr)->symtype) {
 	case SYMBOL_VALUE:
-		(*expr)->dtype = ctx->ectx.dtype;
+		(*expr)->dtype = datatype_get(ctx->ectx.dtype);
 		erec = symbol_parse(*expr, &new);
 		if (erec != NULL) {
 			erec_queue(erec, ctx->msgs);
@@ -312,7 +312,7 @@ static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 
 	prefix = prefix_expr_alloc(&expr->location, value,
 				   datalen * BITS_PER_BYTE);
-	prefix->dtype = ctx->ectx.dtype;
+	prefix->dtype = datatype_get(ctx->ectx.dtype);
 	prefix->flags |= EXPR_F_CONSTANT;
 	prefix->byteorder = BYTEORDER_HOST_ENDIAN;
 
@@ -915,7 +915,7 @@ static int expr_evaluate_range(struct eval_ctx *ctx, struct expr **expr)
 		return expr_error(ctx->msgs, range,
 				  "Range has zero or negative size");
 
-	range->dtype = left->dtype;
+	range->dtype = datatype_get(left->dtype);
 	range->flags |= EXPR_F_CONSTANT;
 	return 0;
 }
@@ -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;
@@ -1311,9 +1311,9 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 	switch (map->mappings->etype) {
 	case EXPR_SET:
 		key = constant_expr_alloc(&map->location,
-				 ctx->ectx.dtype,
-				 ctx->ectx.byteorder,
-				 ctx->ectx.len, NULL);
+					  datatype_get(ctx->ectx.dtype),
+					  ctx->ectx.byteorder,
+					  ctx->ectx.len, NULL);
 
 		mappings = implicit_set_declaration(ctx, "__map%d",
 						    key,
@@ -1356,7 +1356,7 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 					 map->mappings->set->key->dtype->desc,
 					 map->map->dtype->desc);
 
-	map->dtype = map->mappings->set->datatype;
+	map->dtype = datatype_get(map->mappings->set->datatype);
 	map->flags |= EXPR_F_CONSTANT;
 
 	/* Data for range lookups needs to be in big endian order */
@@ -1413,7 +1413,7 @@ static void expr_dtype_integer_compatible(struct eval_ctx *ctx,
 	if (ctx->ectx.dtype &&
 	    ctx->ectx.dtype->basetype == &integer_type &&
 	    ctx->ectx.len == 4 * BITS_PER_BYTE) {
-		expr->dtype = ctx->ectx.dtype;
+		expr->dtype = datatype_get(ctx->ectx.dtype);
 		expr->len   = ctx->ectx.len;
 	}
 }
@@ -1554,7 +1554,7 @@ static void binop_transfer_handle_lhs(struct expr **expr)
 	case OP_LSHIFT:
 	case OP_XOR:
 		tmp = expr_get(left->left);
-		tmp->dtype = left->dtype;
+		tmp->dtype = datatype_get(left->dtype);
 		expr_free(left);
 		*expr = tmp;
 		break;
@@ -2965,7 +2965,7 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 					 map->mappings->set->key->dtype->desc,
 					 map->map->dtype->desc);
 
-	map->dtype = map->mappings->set->datatype;
+	map->dtype = datatype_get(map->mappings->set->datatype);
 	map->flags |= EXPR_F_CONSTANT;
 
 	/* Data for range lookups needs to be in big endian order */
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);
diff --git a/src/segtree.c b/src/segtree.c
index a21270a08c46..082f2093068a 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -540,7 +540,8 @@ static void set_insert_interval(struct expr *set, struct seg_tree *tree,
 {
 	struct expr *expr;
 
-	expr = constant_expr_alloc(&internal_location, tree->keytype,
+	expr = constant_expr_alloc(&internal_location,
+				   datatype_get(tree->keytype),
 				   tree->byteorder, tree->keylen, NULL);
 	mpz_set(expr->value, ei->left);
 	expr = set_elem_expr_alloc(&internal_location, expr);
@@ -606,7 +607,7 @@ static void set_elem_add(const struct set *set, struct expr *init, mpz_t value,
 {
 	struct expr *expr;
 
-	expr = constant_expr_alloc(&internal_location, set->key->dtype,
+	expr = constant_expr_alloc(&internal_location, datatype_get(set->key->dtype),
 				   set->key->byteorder, set->key->len, NULL);
 	mpz_set(expr->value, value);
 	expr = set_elem_expr_alloc(&internal_location, expr);
@@ -888,7 +889,7 @@ void interval_map_decompose(struct expr *set)
 			 mpz_cmp_ui(p, 0)) {
 			struct expr *tmp;
 
-			tmp = constant_expr_alloc(&low->location, low->dtype,
+			tmp = constant_expr_alloc(&low->location, datatype_get(low->dtype),
 						  low->byteorder, expr_value(low)->len,
 						  NULL);
 
@@ -964,7 +965,7 @@ void interval_map_decompose(struct expr *set)
 	if (!low) /* no unclosed interval at end */
 		goto out;
 
-	i = constant_expr_alloc(&low->location, low->dtype,
+	i = constant_expr_alloc(&low->location, datatype_get(low->dtype),
 				low->byteorder, expr_value(low)->len, NULL);
 	mpz_init_bitmask(i->value, i->len);
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-12 12:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-12 12:23 [PATCH nft 1/3,v3] src: add reference counter for dynamic datatypes Pablo Neira Ayuso
2019-06-12 12:23 ` [PATCH nft 2/3] datatype: add datatype_set() Pablo Neira Ayuso
2019-06-12 12:23 ` [PATCH nft 3/3] datatype: dtype_clone() should clone flags too Pablo Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).