[PATCH nft,v2 0/7] cache updates

[PATCH nft,v2 0/7] cache updates

[Pablo Neira Ayuso]

Hi,

The following patchset contains cache updates for nft:

Patch #1 resets filtering for each new command

Patch #2 accumulates cache flags for each command, recent patches are
	 relaxing cache requirements which could uncover bugs.

Patch #3 updates objects to use the netlink dump filtering infrastructure
	 to build the cache (

Patch #4 only dumps rules for the given table

Patch #5 updates reset commands to use the cache infrastructure

Patch #6 and #7 extend tests coverage for reset commands.

Pablo Neira Ayuso (7):
  cache: reset filter for each command
  cache: accumulate flags in batch
  cache: add filtering support for objects
  cache: only dump rules for the given table
  cache: consolidate reset command
  tests: shell: cover anonymous set with reset command
  tests: shell: cover reset command with counter and quota

 include/cache.h                               |  12 +-
 include/netlink.h                             |   5 -
 src/cache.c                                   | 201 ++++++++++++++----
 src/evaluate.c                                |   2 +
 src/mnl.c                                     |   7 +-
 src/netlink.c                                 |  78 -------
 src/parser_bison.y                            |   8 +-
 src/rule.c                                    |  48 +----
 tests/shell/testcases/listing/reset_objects   | 104 +++++++++
 .../testcases/rule_management/0011reset_0     |  31 ++-
 10 files changed, 305 insertions(+), 191 deletions(-)
 create mode 100755 tests/shell/testcases/listing/reset_objects

-- 
2.30.2

[PATCH nft,v2 1/7] cache: reset filter for each command

[Pablo Neira Ayuso]

Inconditionally reset filter for each command in the batch, this is safer.

Fixes: 3f1d3912c3a6 ("cache: filter out tables that are not requested")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: new in this series

 src/cache.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index 233147649263..5442da35a129 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -400,6 +400,11 @@ err_name_too_long:
 	return -1;
 }
 
+static void reset_filter(struct nft_cache_filter *filter)
+{
+	memset(&filter->list, 0, sizeof(filter->list));
+}
+
 int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 		       struct list_head *msgs, struct nft_cache_filter *filter,
 		       unsigned int *pflags)
@@ -411,8 +416,7 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 		if (nft_handle_validate(cmd, msgs) < 0)
 			return -1;
 
-		if (filter->list.table && cmd->op != CMD_LIST)
-			memset(&filter->list, 0, sizeof(filter->list));
+		reset_filter(filter);
 
 		switch (cmd->op) {
 		case CMD_ADD:
-- 
2.30.2

[PATCH nft,v2 2/7] cache: accumulate flags in batch

[Pablo Neira Ayuso]

Recent updates are relaxing cache requirements:

  babc6ee8773c ("cache: populate chains on demand from error path")

Flags describe cache requirements for a given batch, accumulate flags
that are inferred from commands in this batch.

Fixes: 7df42800cf89 ("src: single cache_update() call to build cache before evaluation")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: new in this series

 src/cache.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index 5442da35a129..082fd30b462d 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -409,13 +409,14 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 		       struct list_head *msgs, struct nft_cache_filter *filter,
 		       unsigned int *pflags)
 {
-	unsigned int flags = NFT_CACHE_EMPTY;
+	unsigned int flags, batch_flags = NFT_CACHE_EMPTY;
 	struct cmd *cmd;
 
 	list_for_each_entry(cmd, cmds, list) {
 		if (nft_handle_validate(cmd, msgs) < 0)
 			return -1;
 
+		flags = NFT_CACHE_EMPTY;
 		reset_filter(filter);
 
 		switch (cmd->op) {
@@ -439,13 +440,13 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 			flags = evaluate_cache_get(cmd, flags);
 			break;
 		case CMD_RESET:
-			flags |= evaluate_cache_reset(cmd, flags, filter);
+			flags = evaluate_cache_reset(cmd, flags, filter);
 			break;
 		case CMD_LIST:
-			flags |= evaluate_cache_list(nft, cmd, flags, filter);
+			flags = evaluate_cache_list(nft, cmd, flags, filter);
 			break;
 		case CMD_MONITOR:
-			flags |= NFT_CACHE_FULL;
+			flags = NFT_CACHE_FULL;
 			break;
 		case CMD_FLUSH:
 			flags = evaluate_cache_flush(cmd, flags, filter);
@@ -460,8 +461,9 @@ int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
 		default:
 			break;
 		}
+		batch_flags |= flags;
 	}
-	*pflags = flags;
+	*pflags = batch_flags;
 
 	return 0;
 }
-- 
2.30.2

[PATCH nft,v2 3/7] cache: add filtering support for objects

[Pablo Neira Ayuso]

Currently, full ruleset flag is set on to fetch objects.

Follow a similar approach to these patches from Phil:

 de961b930660 ("cache: Filter set list on server side") and
 cb4b07d0b628 ("cache: Support filtering for a specific flowtable")

in preparation to update the reset command to use the cache
infrastructure.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: remove check for if (filter) when evaluating cache requirements,
    filter must be present in reset command, add assert().

 include/cache.h |   2 +
 src/cache.c     | 100 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/include/cache.h b/include/cache.h
index 44e8430ce1fd..c72bedf542dc 100644
--- a/include/cache.h
+++ b/include/cache.h
@@ -54,8 +54,10 @@ struct nft_cache_filter {
 		uint32_t	family;
 		const char	*table;
 		const char	*chain;
+		const char	*obj;
 		const char	*set;
 		const char	*ft;
+		int		obj_type;
 		uint64_t	rule_handle;
 	} list;
 
diff --git a/src/cache.c b/src/cache.c
index 082fd30b462d..3849a4640416 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -190,6 +190,22 @@ static unsigned int evaluate_cache_rename(struct cmd *cmd, unsigned int flags)
 	return flags;
 }
 
+static void obj_filter_setup(const struct cmd *cmd, unsigned int *flags,
+			     struct nft_cache_filter *filter, int type)
+{
+	assert(filter);
+
+	if (cmd->handle.family)
+		filter->list.family = cmd->handle.family;
+	if (cmd->handle.table.name)
+		filter->list.table = cmd->handle.table.name;
+	if (cmd->handle.obj.name)
+		filter->list.obj = cmd->handle.obj.name;
+
+	filter->list.obj_type = type;
+	*flags |= NFT_CACHE_TABLE | NFT_CACHE_OBJECT;
+}
+
 static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 					unsigned int flags,
 					struct nft_cache_filter *filter)
@@ -251,6 +267,37 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 	case CMD_OBJ_FLOWTABLES:
 		flags |= NFT_CACHE_TABLE | NFT_CACHE_FLOWTABLE;
 		break;
+	case CMD_OBJ_COUNTER:
+	case CMD_OBJ_COUNTERS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_COUNTER);
+		break;
+	case CMD_OBJ_QUOTA:
+	case CMD_OBJ_QUOTAS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_QUOTA);
+		break;
+	case CMD_OBJ_CT_HELPER:
+	case CMD_OBJ_CT_HELPERS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_CT_HELPER);
+		break;
+	case CMD_OBJ_LIMIT:
+	case CMD_OBJ_LIMITS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_LIMIT);
+		break;
+	case CMD_OBJ_CT_TIMEOUT:
+	case CMD_OBJ_CT_TIMEOUTS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_CT_TIMEOUT);
+		break;
+	case CMD_OBJ_SECMARK:
+	case CMD_OBJ_SECMARKS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_SECMARK);
+		break;
+	case CMD_OBJ_CT_EXPECT:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_CT_EXPECT);
+		break;
+	case CMD_OBJ_SYNPROXY:
+	case CMD_OBJ_SYNPROXYS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_SYNPROXY);
+		break;
 	case CMD_OBJ_RULESET:
 	default:
 		flags |= NFT_CACHE_FULL;
@@ -782,10 +829,19 @@ struct obj_cache_dump_ctx {
 static int obj_cache_cb(struct nftnl_obj *nlo, void *arg)
 {
 	struct obj_cache_dump_ctx *ctx = arg;
+	const char *obj_table;
 	const char *obj_name;
+	uint32_t obj_family;
 	struct obj *obj;
 	uint32_t hash;
 
+	obj_table = nftnl_obj_get_str(nlo, NFTNL_OBJ_TABLE);
+	obj_family = nftnl_obj_get_u32(nlo, NFTNL_OBJ_FAMILY);
+
+	if (obj_family != ctx->table->handle.family ||
+	    strcmp(obj_table, ctx->table->handle.table.name))
+		return 0;
+
 	obj = netlink_delinearize_obj(ctx->nlctx, nlo);
 	if (!obj)
 		return -1;
@@ -794,6 +850,9 @@ static int obj_cache_cb(struct nftnl_obj *nlo, void *arg)
 	hash = djb_hash(obj_name) % NFT_CACHE_HSIZE;
 	cache_add(&obj->cache, &ctx->table->obj_cache, hash);
 
+	nftnl_obj_list_del(nlo);
+	nftnl_obj_free(nlo);
+
 	return 0;
 }
 
@@ -810,13 +869,27 @@ static int obj_cache_init(struct netlink_ctx *ctx, struct table *table,
 }
 
 static struct nftnl_obj_list *obj_cache_dump(struct netlink_ctx *ctx,
-					     const struct table *table)
+					     const struct nft_cache_filter *filter)
 {
 	struct nftnl_obj_list *obj_list;
+	int type = NFT_OBJECT_UNSPEC;
+	int family = NFPROTO_UNSPEC;
+	const char *table = NULL;
+	const char *obj = NULL;
+	bool dump = true;
 
-	obj_list = mnl_nft_obj_dump(ctx, table->handle.family,
-				    table->handle.table.name, NULL,
-				    0, true, false);
+	if (filter) {
+		family = filter->list.family;
+		if (filter->list.table)
+			table = filter->list.table;
+		if (filter->list.obj) {
+			obj = filter->list.obj;
+			dump = false;
+		}
+		if (filter->list.obj_type)
+			type = filter->list.obj_type;
+	}
+	obj_list = mnl_nft_obj_dump(ctx, family, table, obj, type, dump, false);
 	if (!obj_list) {
                 if (errno == EINTR)
 			return NULL;
@@ -1039,7 +1112,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 	struct nftnl_flowtable_list *ft_list = NULL;
 	struct nftnl_chain_list *chain_list = NULL;
 	struct nftnl_set_list *set_list = NULL;
-	struct nftnl_obj_list *obj_list;
+	struct nftnl_obj_list *obj_list = NULL;
 	struct table *table;
 	struct set *set;
 	int ret = 0;
@@ -1056,6 +1129,13 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 			goto cache_fails;
 		}
 	}
+	if (flags & NFT_CACHE_OBJECT_BIT) {
+		obj_list = obj_cache_dump(ctx, filter);
+		if (!obj_list) {
+			ret = -1;
+			goto cache_fails;
+		}
+	}
 	if (flags & NFT_CACHE_FLOWTABLE_BIT) {
 		ft_list = ft_cache_dump(ctx, filter);
 		if (!ft_list) {
@@ -1108,15 +1188,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 				goto cache_fails;
 		}
 		if (flags & NFT_CACHE_OBJECT_BIT) {
-			obj_list = obj_cache_dump(ctx, table);
-			if (!obj_list) {
-				ret = -1;
-				goto cache_fails;
-			}
 			ret = obj_cache_init(ctx, table, obj_list);
-
-			nftnl_obj_list_free(obj_list);
-
 			if (ret < 0)
 				goto cache_fails;
 		}
@@ -1137,6 +1209,8 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 cache_fails:
 	if (set_list)
 		nftnl_set_list_free(set_list);
+	if (obj_list)
+		nftnl_obj_list_free(obj_list);
 	if (ft_list)
 		nftnl_flowtable_list_free(ft_list);
 
-- 
2.30.2

[PATCH nft,v2 4/7] cache: only dump rules for the given table

[Pablo Neira Ayuso]

Only family is set on in the dump request, set on table and chain
otherwise, rules for the given family are fetched for each existing
table.

Fixes: afbd102211dc ("src: do not use the nft_cache_filter object from mnl.c")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes

 src/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cache.c b/src/cache.c
index 3849a4640416..c36b3ebc0614 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -694,7 +694,7 @@ int rule_cache_dump(struct netlink_ctx *ctx, const struct handle *h,
 		    bool dump, bool reset)
 {
 	struct nftnl_rule_list *rule_cache;
-	const char *table = NULL;
+	const char *table = h->table.name;
 	const char *chain = NULL;
 	uint64_t rule_handle = 0;
 
-- 
2.30.2

[PATCH nft,v2 5/7] cache: consolidate reset command

[Pablo Neira Ayuso]

Reset command does not utilize the cache infrastructure.

This implicitly fixes a crash with anonymous sets because elements are
not fetched. I initially tried to fix it by toggling the missing cache
flags, but then ASAN reports memleaks.

To address these issues relies on Phil's list filtering infrastructure
which updates is expanded to accomodate filtering requirements of the
reset commands, such as 'reset table ip' where only the family is sent
to the kernel.

After this update, tests/shell reports a few inconsistencies between
reset and list commands:

- reset rules chain t c2

  display sets, but it should only list the given chain.

- reset rules table t
  reset rules ip

  do not list elements in the set. In both cases, these are fully
  listing a given table and family, elements should be included.

The consolidation also ensures list and reset will not differ.

A few more notes:

- CMD_OBJ_TABLE is used for:

	rules family table

  from the parser, due to the lack of a better enum, same applies to
  CMD_OBJ_CHAIN.

- CMD_OBJ_ELEMENTS still does not use the cache, but same occurs in
  the CMD_GET command case which needs to be consolidated.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1763
Fixes: 83e0f4402fb7 ("Implement 'reset {set,map,element}' commands")
Fixes: 1694df2de79f ("Implement 'reset rule' and 'reset rules' commands")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes

 include/cache.h                               | 10 ++-
 include/netlink.h                             |  5 --
 src/cache.c                                   | 81 +++++++++++++------
 src/evaluate.c                                |  2 +
 src/mnl.c                                     |  7 +-
 src/netlink.c                                 | 78 ------------------
 src/parser_bison.y                            |  8 +-
 src/rule.c                                    | 48 +----------
 .../testcases/rule_management/0011reset_0     | 10 +--
 9 files changed, 78 insertions(+), 171 deletions(-)

diff --git a/include/cache.h b/include/cache.h
index c72bedf542dc..e6bde6c6bee3 100644
--- a/include/cache.h
+++ b/include/cache.h
@@ -64,6 +64,12 @@ struct nft_cache_filter {
 	struct {
 		struct list_head head;
 	} obj[NFT_CACHE_HSIZE];
+
+	struct {
+		bool		obj;
+		bool		rule;
+		bool		elem;
+	} reset;
 };
 
 struct nft_cache;
@@ -149,8 +155,4 @@ struct netlink_ctx;
 void nft_chain_cache_update(struct netlink_ctx *ctx, struct table *table,
 			    const char *chain);
 
-int rule_cache_dump(struct netlink_ctx *ctx, const struct handle *h,
-		    const struct nft_cache_filter *filter,
-		    bool dump, bool reset);
-
 #endif /* _NFT_CACHE_H_ */
diff --git a/include/netlink.h b/include/netlink.h
index 27a62462bf7d..cf7ba3693885 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -176,8 +176,6 @@ extern int netlink_delinearize_setelem(struct nftnl_set_elem *nlse,
 				       struct nft_cache *cache);
 
 extern int netlink_list_objs(struct netlink_ctx *ctx, const struct handle *h);
-extern int netlink_reset_objs(struct netlink_ctx *ctx, const struct cmd *cmd,
-			      uint32_t type, bool dump);
 extern struct obj *netlink_delinearize_obj(struct netlink_ctx *ctx,
 					   struct nftnl_obj *nlo);
 
@@ -186,9 +184,6 @@ extern int netlink_list_flowtables(struct netlink_ctx *ctx,
 extern struct flowtable *netlink_delinearize_flowtable(struct netlink_ctx *ctx,
 						       struct nftnl_flowtable *nlo);
 
-extern int netlink_reset_rules(struct netlink_ctx *ctx, const struct cmd *cmd,
-			       bool dump);
-
 extern void netlink_dump_chain(const struct nftnl_chain *nlc,
 			       struct netlink_ctx *ctx);
 extern void netlink_dump_rule(const struct nftnl_rule *nlr,
diff --git a/src/cache.c b/src/cache.c
index c36b3ebc0614..72f2972f0259 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -314,27 +314,49 @@ static unsigned int evaluate_cache_list(struct nft_ctx *nft, struct cmd *cmd,
 static unsigned int evaluate_cache_reset(struct cmd *cmd, unsigned int flags,
 					 struct nft_cache_filter *filter)
 {
+	assert(filter);
+
 	switch (cmd->obj) {
+	case CMD_OBJ_TABLE:
+	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_RULES:
 	case CMD_OBJ_RULE:
-		if (filter) {
-			if (cmd->handle.table.name) {
-				filter->list.family = cmd->handle.family;
-				filter->list.table = cmd->handle.table.name;
-			}
-			if (cmd->handle.chain.name)
-				filter->list.chain = cmd->handle.chain.name;
+		if (cmd->handle.table.name) {
+			filter->list.family = cmd->handle.family;
+			filter->list.table = cmd->handle.table.name;
 		}
-		flags |= NFT_CACHE_SET | NFT_CACHE_FLOWTABLE |
-			 NFT_CACHE_OBJECT | NFT_CACHE_CHAIN;
+		if (cmd->handle.chain.name)
+			filter->list.chain = cmd->handle.chain.name;
+		if (cmd->handle.family)
+			filter->list.family = cmd->handle.family;
+		if (cmd->handle.handle.id)
+			filter->list.rule_handle = cmd->handle.handle.id;
+
+		filter->reset.rule = true;
+		flags |= NFT_CACHE_FULL;
+		break;
+	case CMD_OBJ_COUNTER:
+	case CMD_OBJ_COUNTERS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_COUNTER);
+		filter->reset.obj = true;
+		break;
+	case CMD_OBJ_QUOTA:
+	case CMD_OBJ_QUOTAS:
+		obj_filter_setup(cmd, &flags, filter, NFT_OBJECT_QUOTA);
+		filter->reset.obj = true;
 		break;
-	case CMD_OBJ_ELEMENTS:
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
-		flags |= NFT_CACHE_SET;
+		if (cmd->handle.table.name && cmd->handle.set.name) {
+			filter->list.family = cmd->handle.family;
+			filter->list.table = cmd->handle.table.name;
+			filter->list.set = cmd->handle.set.name;
+		}
+		flags |= NFT_CACHE_SETELEM;
+		filter->reset.elem = true;
 		break;
 	default:
-		flags |= NFT_CACHE_TABLE;
+		flags |= NFT_CACHE_FULL;
 		break;
 	}
 	flags |= NFT_CACHE_REFRESH;
@@ -450,6 +472,7 @@ err_name_too_long:
 static void reset_filter(struct nft_cache_filter *filter)
 {
 	memset(&filter->list, 0, sizeof(filter->list));
+	memset(&filter->reset, 0, sizeof(filter->reset));
 }
 
 int nft_cache_evaluate(struct nft_ctx *nft, struct list_head *cmds,
@@ -689,23 +712,32 @@ static int list_rule_cb(struct nftnl_rule *nlr, void *data)
 	return 0;
 }
 
-int rule_cache_dump(struct netlink_ctx *ctx, const struct handle *h,
-		    const struct nft_cache_filter *filter,
-		    bool dump, bool reset)
+static int rule_cache_dump(struct netlink_ctx *ctx, const struct handle *h,
+			   const struct nft_cache_filter *filter)
 {
 	struct nftnl_rule_list *rule_cache;
 	const char *table = h->table.name;
 	const char *chain = NULL;
 	uint64_t rule_handle = 0;
+	int family = h->family;
+	bool dump = true;
 
 	if (filter) {
-		table = filter->list.table;
-		chain = filter->list.chain;
-		rule_handle = filter->list.rule_handle;
+		if (filter->list.table)
+			table = filter->list.table;
+		if (filter->list.chain)
+			chain = filter->list.chain;
+		if (filter->list.rule_handle) {
+			rule_handle = filter->list.rule_handle;
+			dump = false;
+		}
+		if (filter->list.family)
+			family = filter->list.family;
 	}
 
-	rule_cache = mnl_nft_rule_dump(ctx, h->family,
-				       table, chain, rule_handle, dump, reset);
+	rule_cache = mnl_nft_rule_dump(ctx, family,
+				       table, chain, rule_handle, dump,
+				       filter->reset.rule);
 	if (rule_cache == NULL) {
 		if (errno == EINTR)
 			return -1;
@@ -889,7 +921,8 @@ static struct nftnl_obj_list *obj_cache_dump(struct netlink_ctx *ctx,
 		if (filter->list.obj_type)
 			type = filter->list.obj_type;
 	}
-	obj_list = mnl_nft_obj_dump(ctx, family, table, obj, type, dump, false);
+	obj_list = mnl_nft_obj_dump(ctx, family, table, obj, type, dump,
+				    filter->reset.obj);
 	if (!obj_list) {
                 if (errno == EINTR)
 			return NULL;
@@ -1063,7 +1096,7 @@ static int rule_init_cache(struct netlink_ctx *ctx, struct table *table,
 	struct chain *chain;
 	int ret;
 
-	ret = rule_cache_dump(ctx, &table->handle, filter, true, false);
+	ret = rule_cache_dump(ctx, &table->handle, filter);
 
 	list_for_each_entry_safe(rule, nrule, &ctx->list, list) {
 		chain = chain_cache_find(table, rule->handle.chain.name);
@@ -1159,7 +1192,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 					continue;
 
 				ret = netlink_list_setelems(ctx, &set->handle,
-							    set, false);
+							    set, filter->reset.elem);
 				if (ret < 0)
 					goto cache_fails;
 			}
@@ -1172,7 +1205,7 @@ static int cache_init_objects(struct netlink_ctx *ctx, unsigned int flags,
 					continue;
 
 				ret = netlink_list_setelems(ctx, &set->handle,
-							    set, false);
+							    set, filter->reset.elem);
 				if (ret < 0)
 					goto cache_fails;
 			}
diff --git a/src/evaluate.c b/src/evaluate.c
index 0a31c73e4276..593a0140e92a 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5848,6 +5848,8 @@ static int cmd_evaluate_reset(struct eval_ctx *ctx, struct cmd *cmd)
 		return 0;
 	case CMD_OBJ_ELEMENTS:
 		return setelem_evaluate(ctx, cmd);
+	case CMD_OBJ_TABLE:
+	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
 		return cmd_evaluate_list(ctx, cmd);
diff --git a/src/mnl.c b/src/mnl.c
index 12e145204ef5..db53a60b43cb 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1163,8 +1163,11 @@ struct nftnl_table_list *mnl_nft_table_dump(struct netlink_ctx *ctx,
 		if (!nlt)
 			memory_allocation_error();
 
-		nftnl_table_set_u32(nlt, NFTNL_TABLE_FAMILY, family);
-		nftnl_table_set_str(nlt, NFTNL_TABLE_NAME, table);
+		if (family != NFPROTO_UNSPEC)
+			nftnl_table_set_u32(nlt, NFTNL_TABLE_FAMILY, family);
+		if (table)
+			nftnl_table_set_str(nlt, NFTNL_TABLE_NAME, table);
+
 		flags = NLM_F_ACK;
 	}
 
diff --git a/src/netlink.c b/src/netlink.c
index efb0b69939dc..dea95ffa0704 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1769,84 +1769,6 @@ void netlink_dump_flowtable(struct nftnl_flowtable *flo,
 	fprintf(fp, "\n");
 }
 
-static int list_obj_cb(struct nftnl_obj *nls, void *arg)
-{
-	struct netlink_ctx *ctx = arg;
-	struct obj *obj;
-
-	obj = netlink_delinearize_obj(ctx, nls);
-	if (obj == NULL)
-		return -1;
-	list_add_tail(&obj->list, &ctx->list);
-	return 0;
-}
-
-int netlink_reset_objs(struct netlink_ctx *ctx, const struct cmd *cmd,
-		       uint32_t type, bool dump)
-{
-	const struct handle *h = &cmd->handle;
-	struct nftnl_obj_list *obj_cache;
-	int err;
-
-	obj_cache = mnl_nft_obj_dump(ctx, h->family,
-				     h->table.name, h->obj.name, type, dump, true);
-	if (obj_cache == NULL)
-		return -1;
-
-	err = nftnl_obj_list_foreach(obj_cache, list_obj_cb, ctx);
-	nftnl_obj_list_free(obj_cache);
-	return err;
-}
-
-int netlink_reset_rules(struct netlink_ctx *ctx, const struct cmd *cmd,
-		        bool dump)
-{
-	const struct handle *h = &cmd->handle;
-	struct nft_cache_filter f = {
-		.list.table		= h->table.name,
-		.list.chain		= h->chain.name,
-		.list.rule_handle	= h->handle.id,
-	};
-	struct rule *rule, *next, *crule, *cnext;
-	struct table *table;
-	struct chain *chain;
-	int ret;
-
-	ret = rule_cache_dump(ctx, h, &f, dump, true);
-
-	list_for_each_entry_safe(rule, next, &ctx->list, list) {
-		table = table_cache_find(&ctx->nft->cache.table_cache,
-					 rule->handle.table.name,
-					 rule->handle.family);
-		if (!table)
-			continue;
-
-		chain = chain_cache_find(table, rule->handle.chain.name);
-		if (!chain)
-			continue;
-
-		list_del(&rule->list);
-		list_for_each_entry_safe(crule, cnext, &chain->rules, list) {
-			if (crule->handle.handle.id != rule->handle.handle.id)
-				continue;
-
-			list_replace(&crule->list, &rule->list);
-			rule_free(crule);
-			rule = NULL;
-			break;
-		}
-		if (rule) {
-			list_add_tail(&rule->list, &chain->rules);
-		}
-	}
-	list_for_each_entry_safe(rule, next, &ctx->list, list) {
-		list_del(&rule->list);
-		rule_free(rule);
-	}
-
-	return ret;
-}
-
 struct flowtable *
 netlink_delinearize_flowtable(struct netlink_ctx *ctx,
 			      struct nftnl_flowtable *nlo)
diff --git a/src/parser_bison.y b/src/parser_bison.y
index f3368dd3e922..8fbb98bdcd69 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1757,21 +1757,21 @@ reset_cmd		:	COUNTERS	ruleset_spec
 			}
 			|	RULES		table_spec
 			{
-				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_RULES, &$2, &@$, NULL);
+				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_TABLE, &$2, &@$, NULL);
 			}
 			|	RULES		TABLE	table_spec
 			{
 				/* alias of previous rule. */
-				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_RULES, &$3, &@$, NULL);
+				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_TABLE, &$3, &@$, NULL);
 			}
 			|	RULES		chain_spec
 			{
-				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_RULES, &$2, &@$, NULL);
+				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_CHAIN, &$2, &@$, NULL);
 			}
 			|	RULES		CHAIN	chain_spec
 			{
 				/* alias of previous rule. */
-				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_RULES, &$3, &@$, NULL);
+				$$ = cmd_alloc(CMD_RESET, CMD_OBJ_CHAIN, &$3, &@$, NULL);
 			}
 			|	RULE		ruleid_spec
 			{
diff --git a/src/rule.c b/src/rule.c
index 0f92ef532ece..9bc160ec0d88 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2462,58 +2462,12 @@ static int do_command_get(struct netlink_ctx *ctx, struct cmd *cmd)
 
 static int do_command_reset(struct netlink_ctx *ctx, struct cmd *cmd)
 {
-	struct obj *obj, *next;
-	struct table *table;
-	bool dump = false;
-	uint32_t type;
-	int ret;
-
 	switch (cmd->obj) {
-	case CMD_OBJ_COUNTERS:
-		dump = true;
-		/* fall through */
-	case CMD_OBJ_COUNTER:
-		type = NFT_OBJECT_COUNTER;
-		break;
-	case CMD_OBJ_QUOTAS:
-		dump = true;
-		/* fall through */
-	case CMD_OBJ_QUOTA:
-		type = NFT_OBJECT_QUOTA;
-		break;
-	case CMD_OBJ_RULES:
-		ret = netlink_reset_rules(ctx, cmd, true);
-		if (ret < 0)
-			return ret;
-
-		return do_command_list(ctx, cmd);
-	case CMD_OBJ_RULE:
-		return netlink_reset_rules(ctx, cmd, false);
 	case CMD_OBJ_ELEMENTS:
 		return do_get_setelems(ctx, cmd, true);
-	case CMD_OBJ_SET:
-	case CMD_OBJ_MAP:
-		ret = netlink_list_setelems(ctx, &cmd->handle, cmd->set, true);
-		if (ret < 0)
-			return ret;
-
-		return do_command_list(ctx, cmd);
 	default:
-		BUG("invalid command object type %u\n", cmd->obj);
-	}
-
-	ret = netlink_reset_objs(ctx, cmd, type, dump);
-	list_for_each_entry_safe(obj, next, &ctx->list, list) {
-		table = table_cache_find(&ctx->nft->cache.table_cache,
-					 obj->handle.table.name,
-					 obj->handle.family);
-		if (!obj_cache_find(table, obj->handle.obj.name, obj->type)) {
-			list_del(&obj->list);
-			obj_cache_add(obj, table);
-		}
+		break;
 	}
-	if (ret < 0)
-		return ret;
 
 	return do_command_list(ctx, cmd);
 }
diff --git a/tests/shell/testcases/rule_management/0011reset_0 b/tests/shell/testcases/rule_management/0011reset_0
index 33eadd9eb562..3fede56fb7d8 100755
--- a/tests/shell/testcases/rule_management/0011reset_0
+++ b/tests/shell/testcases/rule_management/0011reset_0
@@ -74,13 +74,6 @@ $DIFF -u <(echo "$EXPECT") <($NFT list ruleset)
 
 echo "resetting specific chain"
 EXPECT='table ip t {
-	set s {
-		type ipv4_addr
-		size 65535
-		flags dynamic
-		counter
-	}
-
 	chain c2 {
 		counter packets 3 bytes 13 accept
 		counter packets 4 bytes 14 drop
@@ -95,6 +88,7 @@ EXPECT='table ip t {
 		size 65535
 		flags dynamic
 		counter
+		elements = { 1.1.1.1 counter packets 1 bytes 11 }
 	}
 
 	chain c {
@@ -116,6 +110,7 @@ EXPECT='table ip t {
 		size 65535
 		flags dynamic
 		counter
+		elements = { 1.1.1.1 counter packets 1 bytes 11 }
 	}
 
 	chain c {
@@ -143,6 +138,7 @@ EXPECT='table ip t {
 		size 65535
 		flags dynamic
 		counter
+		elements = { 1.1.1.1 counter packets 1 bytes 11 }
 	}
 
 	chain c {
-- 
2.30.2

Re: [PATCH nft,v2 5/7] cache: consolidate reset command

[Phil Sutter]

Hi Pablo,

On Mon, Aug 26, 2024 at 10:54:53AM +0200, Pablo Neira Ayuso wrote:
> Reset command does not utilize the cache infrastructure.

This commit changes audit log output for some reason. At least I see
tools/testing/selftests/net/netfilter/nft_audit.sh failing and git
bisect pointed at it. The relevant kselftest output is:

# testing for cmd: nft reset rules ... FAIL
#  table=t1 family=2 entries=3 op=nft_reset_rule
#  table=t2 family=2 entries=3 op=nft_reset_rule
#  table=t2 family=2 entries=3 op=nft_reset_rule
# -table=t2 family=2 entries=180 op=nft_reset_rule
# +table=t2 family=2 entries=186 op=nft_reset_rule
#  table=t2 family=2 entries=188 op=nft_reset_rule
# -table=t2 family=2 entries=135 op=nft_reset_rule
# +table=t2 family=2 entries=129 op=nft_reset_rule

I don't know why entries value changes and whether it is expected or
not. Could you perhaps have a look?

Cheers, Phil

Re: [PATCH nft,v2 5/7] cache: consolidate reset command

[Pablo Neira Ayuso]

On Thu, Sep 26, 2024 at 12:47:30AM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Mon, Aug 26, 2024 at 10:54:53AM +0200, Pablo Neira Ayuso wrote:
> > Reset command does not utilize the cache infrastructure.
> 
> This commit changes audit log output for some reason. At least I see
> tools/testing/selftests/net/netfilter/nft_audit.sh failing and git
> bisect pointed at it. The relevant kselftest output is:
> 
> # testing for cmd: nft reset rules ... FAIL
> #  table=t1 family=2 entries=3 op=nft_reset_rule
> #  table=t2 family=2 entries=3 op=nft_reset_rule
> #  table=t2 family=2 entries=3 op=nft_reset_rule
> # -table=t2 family=2 entries=180 op=nft_reset_rule
> # +table=t2 family=2 entries=186 op=nft_reset_rule
> #  table=t2 family=2 entries=188 op=nft_reset_rule
> # -table=t2 family=2 entries=135 op=nft_reset_rule
> # +table=t2 family=2 entries=129 op=nft_reset_rule
> 
> I don't know why entries value changes and whether it is expected or
> not. Could you perhaps have a look?

Before my patch, there is a single dump request to the kernel:

  rule_cache_dump table (null) chain (null) rule_handle 0 dump 1 reset 1

the skbuff already contains 6 entries for t1/c1 and t1/c2, which why
180 entries of t2 fit into the skbuff is delivered to userspace:

  table=t2 family=2 entries=180 op=nft_reset_rule

(it seems 186 rule entries can fit into the skbuff).

after my patch, there is one for each table:

  rule_cache_dump table t1 chain (null) rule_handle 0 dump 1 reset 1
  rule_cache_dump table t2 chain (null) rule_handle 0 dump 1 reset 1

the skbuff is empty when dumping t2, so we can fit 6 more entries:

table=t2 family=2 entries=186 op=nft_reset_rule

If you take the number, before patch:

180+188+135=503

after patch:

186+188+129=503

show that is the same number of entries. Behaviour is correct.

I don't know how to fix this test without removing the check for
'reset rules', because it will break between different nftables
versions (due to the different strategies to dump all rules vs. dump
rules per table).

And I don't think it makes sense to revert my userspace update just to
make this test happy.

Re: [PATCH nft,v2 5/7] cache: consolidate reset command

[Phil Sutter]

On Thu, Sep 26, 2024 at 04:34:46PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 26, 2024 at 12:47:30AM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Mon, Aug 26, 2024 at 10:54:53AM +0200, Pablo Neira Ayuso wrote:
> > > Reset command does not utilize the cache infrastructure.
> > 
> > This commit changes audit log output for some reason. At least I see
> > tools/testing/selftests/net/netfilter/nft_audit.sh failing and git
> > bisect pointed at it. The relevant kselftest output is:
> > 
> > # testing for cmd: nft reset rules ... FAIL
> > #  table=t1 family=2 entries=3 op=nft_reset_rule
> > #  table=t2 family=2 entries=3 op=nft_reset_rule
> > #  table=t2 family=2 entries=3 op=nft_reset_rule
> > # -table=t2 family=2 entries=180 op=nft_reset_rule
> > # +table=t2 family=2 entries=186 op=nft_reset_rule
> > #  table=t2 family=2 entries=188 op=nft_reset_rule
> > # -table=t2 family=2 entries=135 op=nft_reset_rule
> > # +table=t2 family=2 entries=129 op=nft_reset_rule
> > 
> > I don't know why entries value changes and whether it is expected or
> > not. Could you perhaps have a look?
> 
> Before my patch, there is a single dump request to the kernel:
> 
>   rule_cache_dump table (null) chain (null) rule_handle 0 dump 1 reset 1
> 
> the skbuff already contains 6 entries for t1/c1 and t1/c2, which why
> 180 entries of t2 fit into the skbuff is delivered to userspace:
> 
>   table=t2 family=2 entries=180 op=nft_reset_rule
> 
> (it seems 186 rule entries can fit into the skbuff).
> 
> after my patch, there is one for each table:
> 
>   rule_cache_dump table t1 chain (null) rule_handle 0 dump 1 reset 1
>   rule_cache_dump table t2 chain (null) rule_handle 0 dump 1 reset 1
> 
> the skbuff is empty when dumping t2, so we can fit 6 more entries:
> 
> table=t2 family=2 entries=186 op=nft_reset_rule
> 
> If you take the number, before patch:
> 
> 180+188+135=503
> 
> after patch:
> 
> 186+188+129=503
> 
> show that is the same number of entries. Behaviour is correct.

Thanks for investigating!

> I don't know how to fix this test without removing the check for
> 'reset rules', because it will break between different nftables
> versions (due to the different strategies to dump all rules vs. dump
> rules per table).
> 
> And I don't think it makes sense to revert my userspace update just to
> make this test happy.

Maybe a valid measure is to sum up entries there to make sure the total
remains the same (just like you did above). I'll have a look at making
the test tolerant to both variants.

Thanks, Phil

[PATCH nft,v2 6/7] tests: shell: cover anonymous set with reset command

[Pablo Neira Ayuso]

Extend existing test to reset counters for rules with anonymous set.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1763
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes

 .../testcases/rule_management/0011reset_0     | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/shell/testcases/rule_management/0011reset_0 b/tests/shell/testcases/rule_management/0011reset_0
index 3fede56fb7d8..2004b17d5822 100755
--- a/tests/shell/testcases/rule_management/0011reset_0
+++ b/tests/shell/testcases/rule_management/0011reset_0
@@ -4,6 +4,27 @@
 
 set -e
 
+echo "loading ruleset with anonymous set"
+$NFT -f - <<EOF
+table t {
+        chain dns-nat-pre {
+                type nat hook prerouting priority filter; policy accept;
+                meta l4proto { tcp, udp } th dport 53 ip saddr 10.24.0.0/24 ip daddr != 10.25.0.1 counter packets 1000 bytes 1000 dnat to 10.25.0.1
+        }
+}
+EOF
+
+echo "resetting ruleset with anonymous set"
+$NFT reset rules
+EXPECT='table ip t {
+	chain dns-nat-pre {
+		type nat hook prerouting priority filter; policy accept;
+		meta l4proto { tcp, udp } th dport 53 ip saddr 10.24.0.0/24 ip daddr != 10.25.0.1 counter packets 0 bytes 0 dnat to 10.25.0.1
+	}
+}'
+$DIFF -u <(echo "$EXPECT") <($NFT list ruleset)
+$NFT flush ruleset
+
 echo "loading ruleset"
 $NFT -f - <<EOF
 table ip t {
-- 
2.30.2

[PATCH nft,v2 7/7] tests: shell: cover reset command with counter and quota

[Pablo Neira Ayuso]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes

 tests/shell/testcases/listing/reset_objects | 104 ++++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100755 tests/shell/testcases/listing/reset_objects

diff --git a/tests/shell/testcases/listing/reset_objects b/tests/shell/testcases/listing/reset_objects
new file mode 100755
index 000000000000..0b6720b62c04
--- /dev/null
+++ b/tests/shell/testcases/listing/reset_objects
@@ -0,0 +1,104 @@
+#!/bin/bash
+
+set -e
+
+load_ruleset()
+{
+	$NFT -f - <<EOF
+table ip test {
+	quota https-quota {
+		25 mbytes used 10 mbytes
+	}
+	counter https-counter {
+		packets 10 bytes 4096
+	}
+}
+EOF
+}
+
+check_list_quota()
+{
+	EXPECT="table ip test {
+	quota https-quota {
+		25 mbytes
+	}
+}"
+	$DIFF -u <(echo "$EXPECT") <($NFT list quotas)
+}
+
+check_list_counter()
+{
+	EXPECT="table ip test {
+	counter https-counter {
+		packets 0 bytes 0
+	}
+}"
+	$DIFF -u <(echo "$EXPECT") <($NFT list counters)
+}
+
+load_ruleset
+
+EXPECT="table ip test {
+	quota https-quota {
+		25 mbytes used 10 mbytes
+	}
+}"
+$DIFF -u <(echo "$EXPECT") <($NFT reset quotas)
+
+check_list_quota
+$NFT flush ruleset
+load_ruleset
+
+EXPECT="table ip test {
+	quota https-quota {
+		25 mbytes used 10 mbytes
+	}
+}"
+$DIFF -u <(echo "$EXPECT") <($NFT reset quotas ip)
+
+check_list_quota
+$NFT flush ruleset
+load_ruleset
+
+EXPECT="table ip test {
+	quota https-quota {
+		25 mbytes used 10 mbytes
+	}
+}"
+$DIFF -u <(echo "$EXPECT") <($NFT reset quota ip test https-quota)
+
+check_list_quota
+$NFT flush ruleset
+load_ruleset
+
+EXPECT="table ip test {
+	counter https-counter {
+		packets 10 bytes 4096
+	}
+}"
+$DIFF -u <(echo "$EXPECT") <($NFT reset counters)
+
+check_list_counter
+$NFT flush ruleset
+load_ruleset
+
+EXPECT="table ip test {
+	counter https-counter {
+		packets 10 bytes 4096
+	}
+}"
+$DIFF -u <(echo "$EXPECT") <($NFT reset counters ip)
+
+check_list_counter
+$NFT flush ruleset
+load_ruleset
+
+EXPECT="table ip test {
+	counter https-counter {
+		packets 10 bytes 4096
+	}
+}"
+$DIFF -u <(echo "$EXPECT") <($NFT reset counter ip test https-counter)
+
+check_list_counter
+$NFT flush ruleset
-- 
2.30.2

Re: [PATCH nft,v2 0/7] cache updates

[Eric Garver]

On Mon, Aug 26, 2024 at 10:54:48AM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> The following patchset contains cache updates for nft:
> 
> Patch #1 resets filtering for each new command
> 
> Patch #2 accumulates cache flags for each command, recent patches are
> 	 relaxing cache requirements which could uncover bugs.
> 
> Patch #3 updates objects to use the netlink dump filtering infrastructure
> 	 to build the cache (
> 
> Patch #4 only dumps rules for the given table
> 
> Patch #5 updates reset commands to use the cache infrastructure
> 
> Patch #6 and #7 extend tests coverage for reset commands.
> 
> Pablo Neira Ayuso (7):
>   cache: reset filter for each command
>   cache: accumulate flags in batch
>   cache: add filtering support for objects
>   cache: only dump rules for the given table
>   cache: consolidate reset command
>   tests: shell: cover anonymous set with reset command
>   tests: shell: cover reset command with counter and quota
> 
>  include/cache.h                               |  12 +-
>  include/netlink.h                             |   5 -
>  src/cache.c                                   | 201 ++++++++++++++----
>  src/evaluate.c                                |   2 +
>  src/mnl.c                                     |   7 +-
>  src/netlink.c                                 |  78 -------
>  src/parser_bison.y                            |   8 +-
>  src/rule.c                                    |  48 +----
>  tests/shell/testcases/listing/reset_objects   | 104 +++++++++
>  .../testcases/rule_management/0011reset_0     |  31 ++-
>  10 files changed, 305 insertions(+), 191 deletions(-)
>  create mode 100755 tests/shell/testcases/listing/reset_objects
> 
> -- 
> 2.30.2

I ran this against the firewalld testsuite; lgtm. It does not cover
"reset" commands.

Tested-by: Eric Garver <eric@garver.life>

Re: [PATCH nft,v2 0/7] cache updates

[Pablo Neira Ayuso]

On Mon, Aug 26, 2024 at 11:29:20AM -0400, Eric Garver wrote:
> On Mon, Aug 26, 2024 at 10:54:48AM +0200, Pablo Neira Ayuso wrote:
> > Hi,
> > 
> > The following patchset contains cache updates for nft:
> > 
> > Patch #1 resets filtering for each new command
> > 
> > Patch #2 accumulates cache flags for each command, recent patches are
> > 	 relaxing cache requirements which could uncover bugs.
> > 
> > Patch #3 updates objects to use the netlink dump filtering infrastructure
> > 	 to build the cache (
> > 
> > Patch #4 only dumps rules for the given table
> > 
> > Patch #5 updates reset commands to use the cache infrastructure
> > 
> > Patch #6 and #7 extend tests coverage for reset commands.
[...]
> I ran this against the firewalld testsuite; lgtm. It does not cover
> "reset" commands.
> 
> Tested-by: Eric Garver <eric@garver.life>

Pushed out, thanks for testing.

I have one more series cooking for cache refining, I will get on Cc.