packages/nftables
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
nftables/backport-datatype-fix-leak-and-cleanup-reference-counting-for.patch
lingsheng 7a10caa70e backport upstream patches 
Signed-off-by: lingsheng <860373352@qq.com>
(cherry picked from commit 757a6bc4a7db26ad71cdecec0548ca6250ceaffc)
2024-04-19 10:15:35 +08:00

431 lines
14 KiB
Diff
Raw Permalink Blame History

From 8519ab031d8022999603a69ee9f18e8cfb06645d Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Tue, 12 Sep 2023 09:30:54 +0200
Subject: [PATCH] datatype: fix leak and cleanup reference counting for struct
datatype
Test `./tests/shell/run-tests.sh -V tests/shell/testcases/maps/nat_addr_port`
fails:
==118== 195 (112 direct, 83 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==118== at 0x484682C: calloc (vg_replace_malloc.c:1554)
==118== by 0x48A39DD: xmalloc (utils.c:37)
==118== by 0x48A39DD: xzalloc (utils.c:76)
==118== by 0x487BDFD: datatype_alloc (datatype.c:1205)
==118== by 0x487BDFD: concat_type_alloc (datatype.c:1288)
==118== by 0x488229D: stmt_evaluate_nat_map (evaluate.c:3786)
==118== by 0x488229D: stmt_evaluate_nat (evaluate.c:3892)
==118== by 0x488229D: stmt_evaluate (evaluate.c:4450)
==118== by 0x488328E: rule_evaluate (evaluate.c:4956)
==118== by 0x48ADC71: nft_evaluate (libnftables.c:552)
==118== by 0x48AEC29: nft_run_cmd_from_buffer (libnftables.c:595)
==118== by 0x402983: main (main.c:534)
I think the reference handling for datatype is wrong. It was introduced
by commit 01a13882bb59 ('src: add reference counter for dynamic
datatypes').
We don't notice it most of the time, because instances are statically
allocated, where datatype_get()/datatype_free() is a NOP.
Fix and rework.
- Commit 01a13882bb59 comments "The reference counter of any newly
allocated datatype is set to zero". That seems not workable.
Previously, functions like datatype_clone() would have returned the
refcnt set to zero. Some callers would then then set the refcnt to one, but
some wouldn't (set_datatype_alloc()). Calling datatype_free() with a
refcnt of zero will overflow to UINT_MAX and leak:
if (--dtype->refcnt > 0)
return;
While there could be schemes with such asymmetric counting that juggle the
appropriate number of datatype_get() and datatype_free() calls, this is
confusing and error prone. The common pattern is that every
alloc/clone/get/ref is paired with exactly one unref/free.
Let datatype_clone() return references with refcnt set 1 and in
general be always clear about where we transfer ownership (take a
reference) and where we need to release it.
- set_datatype_alloc() needs to consistently return ownership to the
reference. Previously, some code paths would and others wouldn't.
- Replace
datatype_set(key, set_datatype_alloc(dtype, key->byteorder))
with a __datatype_set() with takes ownership.
Fixes: 01a13882bb59 ('src: add reference counter for dynamic datatypes')
Signed-off-by: Thomas Haller <thaller@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/datatype.h | 1 +
include/expression.h | 4 +++
src/datatype.c | 23 ++++++++++----
src/evaluate.c | 47 ++++++++++++++++++++++++---------------
src/expression.c | 2 +-
src/netlink.c | 31 +++++++++---------
src/netlink_delinearize.c | 2 +-
7 files changed, 70 insertions(+), 40 deletions(-)
diff --git a/include/datatype.h b/include/datatype.h
index 6146eda1..52a2e943 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -175,6 +175,7 @@ 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_set(struct expr *expr, const struct datatype *dtype);
+extern void __datatype_set(struct expr *expr, const struct datatype *dtype);
extern void datatype_free(const struct datatype *dtype);
struct parse_ctx {
diff --git a/include/expression.h b/include/expression.h
index 733dd3cf..469f41ec 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -116,7 +116,11 @@ enum symbol_types {
* @maxval: expected maximum value
*/
struct expr_ctx {
+ /* expr_ctx does not own the reference to dtype. The caller must ensure
+ * the valid lifetime.
+ */
const struct datatype *dtype;
+
enum byteorder byteorder;
unsigned int len;
unsigned int maxval;
diff --git a/src/datatype.c b/src/datatype.c
index 678a16b1..70c84846 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1078,6 +1078,7 @@ static struct datatype *dtype_alloc(void)
dtype = xzalloc(sizeof(*dtype));
dtype->flags = DTYPE_F_ALLOC;
+ dtype->refcnt = 1;
return dtype;
}
@@ -1095,12 +1096,19 @@ struct datatype *datatype_get(const struct datatype *ptr)
return dtype;
}
+void __datatype_set(struct expr *expr, const struct datatype *dtype)
+{
+ const struct datatype *dtype_free;
+
+ dtype_free = expr->dtype;
+ expr->dtype = dtype;
+ datatype_free(dtype_free);
+}
+
void datatype_set(struct expr *expr, const struct datatype *dtype)
{
- if (dtype == expr->dtype)
- return;
- datatype_free(expr->dtype);
- expr->dtype = datatype_get(dtype);
+ if (dtype != expr->dtype)
+ __datatype_set(expr, datatype_get(dtype));
}
static struct datatype *dtype_clone(const struct datatype *orig_dtype)
@@ -1112,7 +1120,7 @@ static struct datatype *dtype_clone(const struct datatype *orig_dtype)
dtype->name = xstrdup(orig_dtype->name);
dtype->desc = xstrdup(orig_dtype->desc);
dtype->flags = DTYPE_F_ALLOC | orig_dtype->flags;
- dtype->refcnt = 0;
+ dtype->refcnt = 1;
return dtype;
}
@@ -1125,6 +1133,9 @@ void datatype_free(const struct datatype *ptr)
return;
if (!(dtype->flags & DTYPE_F_ALLOC))
return;
+
+ assert(dtype->refcnt != 0);
+
if (--dtype->refcnt > 0)
return;
@@ -1177,7 +1188,7 @@ const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
/* Restrict dynamic datatype allocation to generic integer datatype. */
if (orig_dtype != &integer_type)
- return orig_dtype;
+ return datatype_get(orig_dtype);
dtype = dtype_clone(orig_dtype);
dtype->byteorder = byteorder;
diff --git a/src/evaluate.c b/src/evaluate.c
index b0c6919f..1b7e0b37 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -74,7 +74,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
if (dtype->byteorder == key->byteorder)
return;
- datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
+ __datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
}
static int set_evaluate(struct eval_ctx *ctx, struct set *set);
@@ -1289,7 +1289,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
}
(*expr)->flags |= flags;
- datatype_set(*expr, concat_type_alloc(ntype));
+ __datatype_set(*expr, concat_type_alloc(ntype));
(*expr)->len = (*expr)->dtype->size;
if (off > 0)
@@ -1519,7 +1519,6 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
{
struct expr_ctx ectx = ctx->ectx;
struct expr *map = *expr, *mappings;
- const struct datatype *dtype;
struct expr *key, *data;
if (map->map->etype == EXPR_CT &&
@@ -1555,9 +1555,11 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
ctx->ectx.byteorder,
ctx->ectx.len, NULL);
+ const struct datatype *dtype;
dtype = set_datatype_alloc(ectx.dtype, ectx.byteorder);
data = constant_expr_alloc(&netlink_location, dtype,
dtype->byteorder, ectx.len, NULL);
+ datatype_free(dtype);
mappings = implicit_set_declaration(ctx, "__map%d",
key, data,
@@ -3203,8 +3202,10 @@ static int stmt_evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt,
static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
{
struct expr *one, *two, *data, *tmp;
- const struct datatype *dtype;
- int addr_type, err;
+ const struct datatype *dtype = NULL;
+ const struct datatype *dtype2;
+ int addr_type;
+ int err;
switch (stmt->nat.family) {
case NFPROTO_IPV4:
@@ -3219,11 +3220,15 @@ static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
dtype = concat_type_alloc((addr_type << TYPE_BITS) | TYPE_INET_SERVICE);
expr_set_context(&ctx->ectx, dtype, dtype->size);
- if (expr_evaluate(ctx, &stmt->nat.addr))
- return -1;
+ if (expr_evaluate(ctx, &stmt->nat.addr)) {
+ err = -1;
+ goto out;
+ }
- if (stmt->nat.addr->etype != EXPR_MAP)
- return 0;
+ if (stmt->nat.addr->etype != EXPR_MAP) {
+ err = 0;
+ goto out;
+ }
data = stmt->nat.addr->mappings->set->data;
if (data->flags & EXPR_F_INTERVAL)
@@ -3231,36 +3236,42 @@ static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
datatype_set(data, dtype);
- if (expr_ops(data)->type != EXPR_CONCAT)
- return __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
+ if (expr_ops(data)->type != EXPR_CONCAT) {
+ err = __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
BYTEORDER_BIG_ENDIAN,
&stmt->nat.addr);
+ goto out;
+ }
one = list_first_entry(&data->expressions, struct expr, list);
two = list_entry(one->list.next, struct expr, list);
- if (one == two || !list_is_last(&two->list, &data->expressions))
- return __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
+ if (one == two || !list_is_last(&two->list, &data->expressions)) {
+ err = __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
BYTEORDER_BIG_ENDIAN,
&stmt->nat.addr);
+ goto out;
+ }
- dtype = get_addr_dtype(stmt->nat.family);
+ dtype2 = get_addr_dtype(stmt->nat.family);
tmp = one;
- err = __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
+ err = __stmt_evaluate_arg(ctx, stmt, dtype2, dtype2->size,
BYTEORDER_BIG_ENDIAN,
&tmp);
if (err < 0)
- return err;
+ goto out;
if (tmp != one)
BUG("Internal error: Unexpected alteration of l3 expression");
tmp = two;
err = nat_evaluate_transport(ctx, stmt, &tmp);
if (err < 0)
- return err;
+ goto out;
if (tmp != two)
BUG("Internal error: Unexpected alteration of l4 expression");
+out:
+ datatype_free(dtype);
return err;
}
@@ -3941,7 +3952,7 @@ static int set_expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
}
(*expr)->flags |= flags;
- datatype_set(*expr, concat_type_alloc(ntype));
+ __datatype_set(*expr, concat_type_alloc(ntype));
(*expr)->len = (*expr)->dtype->size;
expr_set_context(&ctx->ectx, (*expr)->dtype, (*expr)->len);
diff --git a/src/expression.c b/src/expression.c
index cb222a2b..87d5a9fc 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -997,7 +997,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
if (!dtype)
goto err_free;
- concat_expr->dtype = datatype_get(dtype);
+ __datatype_set(concat_expr, dtype);
concat_expr->len = dtype->size;
return concat_expr;
diff --git a/src/netlink.c b/src/netlink.c
index 59cde9a4..4d3c1cf1 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -740,6 +740,10 @@ enum nft_data_types dtype_map_to_kernel(const struct datatype *dtype)
static const struct datatype *dtype_map_from_kernel(enum nft_data_types type)
{
+ /* The function always returns ownership of a reference. But for
+ * &verdict_Type and datatype_lookup(), those are static instances,
+ * we can omit the datatype_get() call.
+ */
switch (type) {
case NFT_DATA_VERDICT:
return &verdict_type;
@@ -875,12 +879,14 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
const struct nftnl_udata *ud[NFTNL_UDATA_SET_MAX + 1] = {};
enum byteorder keybyteorder = BYTEORDER_INVALID;
enum byteorder databyteorder = BYTEORDER_INVALID;
- const struct datatype *keytype, *datatype = NULL;
struct expr *typeof_expr_key, *typeof_expr_data;
struct setelem_parse_ctx set_parse_ctx;
+ const struct datatype *datatype = NULL;
+ const struct datatype *keytype = NULL;
+ const struct datatype *dtype2 = NULL;
+ const struct datatype *dtype = NULL;
const char *udata, *comment = NULL;
uint32_t flags, key, objtype = 0;
- const struct datatype *dtype;
uint32_t data_interval = 0;
bool automerge = false;
struct set *set;
@@ -932,8 +938,8 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
netlink_io_error(ctx, NULL,
"Unknown data type in set key %u",
data);
- datatype_free(keytype);
- return NULL;
+ set = NULL;
+ goto out;
}
}
@@ -969,16 +975,15 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
list_splice_tail(&set_parse_ctx.stmt_list, &set->stmt_list);
if (datatype) {
- dtype = set_datatype_alloc(datatype, databyteorder);
+ dtype2 = set_datatype_alloc(datatype, databyteorder);
klen = nftnl_set_get_u32(nls, NFTNL_SET_DATA_LEN) * BITS_PER_BYTE;
if (set_udata_key_valid(typeof_expr_data, dtype, klen)) {
- datatype_free(datatype_get(dtype));
set->data = typeof_expr_data;
} else {
expr_free(typeof_expr_data);
set->data = constant_expr_alloc(&netlink_location,
- dtype,
+ dtype2,
databyteorder, klen,
NULL);
@@ -989,16 +994,12 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
if (data_interval)
set->data->flags |= EXPR_F_INTERVAL;
-
- if (dtype != datatype)
- datatype_free(datatype);
}
dtype = set_datatype_alloc(keytype, keybyteorder);
klen = nftnl_set_get_u32(nls, NFTNL_SET_KEY_LEN) * BITS_PER_BYTE;
if (set_udata_key_valid(typeof_expr_key, dtype, klen)) {
- datatype_free(datatype_get(dtype));
set->key = typeof_expr_key;
set->key_typeof_valid = true;
} else {
@@ -1008,9 +1009,6 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
NULL);
}
- if (dtype != keytype)
- datatype_free(keytype);
-
set->flags = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
set->handle.handle.id = nftnl_set_get_u64(nls, NFTNL_SET_HANDLE);
@@ -1038,6 +1036,11 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
}
}
+out:
+ datatype_free(datatype);
+ datatype_free(keytype);
+ datatype_free(dtype2);
+ datatype_free(dtype);
return set;
}
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 19c3f0bd..41cb37a3 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2489,7 +2489,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
ntype = concat_subtype_add(ntype, i->dtype->type);
}
- datatype_set(expr, concat_type_alloc(ntype));
+ __datatype_set(expr, concat_type_alloc(ntype));
break;
}
case EXPR_UNARY:
--
2.33.0
Reference in New Issue View Git Blame Copy Permalink