[PATCH 1/2 nft] src: revisit cache population logic

[PATCH 1/2 nft] src: revisit cache population logic

[Pablo Neira Ayuso]

We get a partial cache (tables, chains and sets) when:

* We see a set reference from a rule, since this set object may be
  already defined in kernelspace and we need to fetch the datatype
  for evaluation.

* We add/delete a set element, we need this to evaluate if the
  element datatype is correct.

* We rename a chain, since we need to know the chain handle.

* We add a chain/set. This isn't needed for simple command line
  invocations. However, since the existing codepath is also exercised
  from `nft -f' context, we need to know if the object exists in the
  kernel. Thus, if this a newly declared object (not yet in the kernel) we
  add it to the cache, otherwise, we will not find follow up references to
  this object in our cache.

We get a full cache when:

* We list the ruleset. We can provide finer grain listing though,
  via partial cache, later.

* We monitor updates, since this displays incremental updates based on
  the existing objects.

* We export the ruleset, since this dumps all of the existing objects.

* We push updates via `nft -f'. We need to know what objects are
  already in the kernel for incremental updates. Otherwise,
  cache_update() hits a bogus 'set doesn't exist' error message for
  just declared set in this batch.  To avoid this problem, we need a
  way to differentiate between what objects in the lists that are
  already defined in the kernel and what are just declared in this
  batch (hint: the location structure information is set for just
  declared objects).

We don't get a cache at all when:

* We flush the ruleset, this is important in case of delinearize
  bugs, so you don't need to reboot or manually flush the ruleset via
  libnftnl examples/nft-table-flush.

* We delete any object, except for set elements (as we describe above).

* We add a rule, so you can generate via --debug=netlink the expression
  without requiring a table and chain in place.

* We describe a expression.

This patch also includes some intentional adjustments to the shell tests
to we don't get bogus errors due to changes in the list printing.

BTW, this patch also includes a revert for 97493717e738 ("evaluate: check
if table and chain exists when adding rules") since that check is not
possible anymore with this logic.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c                           | 61 +++++++++++++++++++++++---------
 src/main.c                               |  4 +++
 tests/shell/testcases/listing/0010sets_0 | 24 ++++++-------
 tests/shell/testcases/listing/0011sets_0 |  4 +--
 4 files changed, 63 insertions(+), 30 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index b17cc82..45d585d 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -165,6 +165,7 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 	struct table *table;
 	struct set *set;
 	struct expr *new;
+	int ret;
 
 	switch ((*expr)->symtype) {
 	case SYMBOL_VALUE:
@@ -184,6 +185,10 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 		new = expr_clone(sym->expr);
 		break;
 	case SYMBOL_SET:
+		ret = cache_update(ctx->cmd->op, ctx->msgs);
+		if (ret < 0)
+			return cmd_error(ctx, "Could not process rule: Cannot list sets");
+
 		table = table_lookup(&ctx->cmd->handle);
 		if (table == NULL)
 			return cmd_error(ctx, "Could not process rule: Table '%s' does not exist",
@@ -2297,27 +2302,30 @@ static int table_evaluate(struct eval_ctx *ctx, struct table *table)
 
 static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 {
-	struct table *table;
+	int ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
+		ret = cache_update(cmd->op, ctx->msgs);
+		if (ret < 0)
+			return ret;
+
 		return setelem_evaluate(ctx, &cmd->expr);
 	case CMD_OBJ_SET:
+		ret = cache_update(cmd->op, ctx->msgs);
+		if (ret < 0)
+			return ret;
+
 		handle_merge(&cmd->set->handle, &cmd->handle);
 		return set_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
 		handle_merge(&cmd->rule->handle, &cmd->handle);
-		table = table_lookup_global(ctx);
-		if (table == NULL)
-			return cmd_error(ctx, "Could not process rule: Table '%s' does not exist",
-					 ctx->cmd->handle.table);
-
-		if (chain_lookup(table, &ctx->cmd->handle) == NULL)
-			return cmd_error(ctx, "Could not process rule: Chain '%s' does not exist",
-					 ctx->cmd->handle.chain);
-
 		return rule_evaluate(ctx, cmd->rule);
 	case CMD_OBJ_CHAIN:
+		ret = cache_update(cmd->op, ctx->msgs);
+		if (ret < 0)
+			return ret;
+
 		return chain_evaluate(ctx, cmd->chain);
 	case CMD_OBJ_TABLE:
 		return table_evaluate(ctx, cmd->table);
@@ -2328,8 +2336,14 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 
 static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 {
+	int ret;
+
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
+		ret = cache_update(cmd->op, ctx->msgs);
+		if (ret < 0)
+			return ret;
+
 		return setelem_evaluate(ctx, &cmd->expr);
 	case CMD_OBJ_SET:
 	case CMD_OBJ_RULE:
@@ -2344,6 +2358,11 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	struct table *table;
+	int ret;
+
+	ret = cache_update(cmd->op, ctx->msgs);
+	if (ret < 0)
+		return ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
@@ -2385,9 +2404,14 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 static int cmd_evaluate_rename(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	struct table *table;
+	int ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_CHAIN:
+		ret = cache_update(cmd->op, ctx->msgs);
+		if (ret < 0)
+			return ret;
+
 		table = table_lookup(&ctx->cmd->handle);
 		if (table == NULL)
 			return cmd_error(ctx, "Could not process rule: Table '%s' does not exist",
@@ -2452,6 +2476,11 @@ static uint32_t monitor_flags[CMD_MONITOR_EVENT_MAX][CMD_MONITOR_OBJ_MAX] = {
 static int cmd_evaluate_monitor(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	uint32_t event;
+	int ret;
+
+	ret = cache_update(cmd->op, ctx->msgs);
+	if (ret < 0)
+		return ret;
 
 	if (cmd->monitor->event == NULL)
 		event = CMD_MONITOR_EVENT_ANY;
@@ -2468,14 +2497,13 @@ static int cmd_evaluate_monitor(struct eval_ctx *ctx, struct cmd *cmd)
 	return 0;
 }
 
-int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
+static int cmd_evaluate_export(struct eval_ctx *ctx, struct cmd *cmd)
 {
-	int ret;
-
-	ret = cache_update(cmd->op, ctx->msgs);
-	if (ret < 0)
-		return ret;
+	return cache_update(cmd->op, ctx->msgs);
+}
 
+int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
+{
 #ifdef DEBUG
 	if (debug_level & DEBUG_EVALUATION) {
 		struct error_record *erec;
@@ -2500,6 +2528,7 @@ int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
 	case CMD_RENAME:
 		return cmd_evaluate_rename(ctx, cmd);
 	case CMD_EXPORT:
+		return cmd_evaluate_export(ctx, cmd);
 	case CMD_DESCRIBE:
 		return 0;
 	case CMD_MONITOR:
diff --git a/src/main.c b/src/main.c
index d643970..ad73d80 100644
--- a/src/main.c
+++ b/src/main.c
@@ -338,6 +338,10 @@ int main(int argc, char * const *argv)
 		scanner = scanner_init(&state);
 		scanner_push_buffer(scanner, &indesc_cmdline, buf);
 	} else if (filename != NULL) {
+		rc = cache_update(CMD_INVALID, &msgs);
+		if (rc < 0)
+			return rc;
+
 		parser_init(&state, &msgs);
 		scanner = scanner_init(&state);
 		if (scanner_read_file(scanner, filename, &internal_location) < 0)
diff --git a/tests/shell/testcases/listing/0010sets_0 b/tests/shell/testcases/listing/0010sets_0
index 42d60b4..855cceb 100755
--- a/tests/shell/testcases/listing/0010sets_0
+++ b/tests/shell/testcases/listing/0010sets_0
@@ -12,18 +12,6 @@ table ip6 test {
 		type ipv6_addr
 	}
 }
-table inet filter {
-	set set0 {
-		type inet_service
-	}
-	set set1 {
-		type inet_service
-		flags constant
-	}
-	set set2 {
-		type icmpv6_type
-	}
-}
 table arp test_arp {
 	set test_set_arp00 {
 		type inet_service
@@ -37,6 +25,18 @@ table bridge test_bridge {
 	set test_set_bridge {
 		type inet_service
 	}
+}
+table inet filter {
+	set set0 {
+		type inet_service
+	}
+	set set1 {
+		type inet_service
+		flags constant
+	}
+	set set2 {
+		type icmpv6_type
+	}
 }"
 
 set -e
diff --git a/tests/shell/testcases/listing/0011sets_0 b/tests/shell/testcases/listing/0011sets_0
index 1bf6887..75f2895 100755
--- a/tests/shell/testcases/listing/0011sets_0
+++ b/tests/shell/testcases/listing/0011sets_0
@@ -6,11 +6,11 @@ EXPECTED="table ip nat {
 }
 table ip6 test {
 }
-table inet filter {
-}
 table arp test_arp {
 }
 table bridge test_bridge {
+}
+table inet filter {
 }"
 
 set -e
-- 
2.1.4

[PATCH 2/2 nft] evaluate: use table_lookup_global() from expr_evaluate_symbol()

[Pablo Neira Ayuso]

If there's already a table 'test' defined in the kernel and you load
another table 'test' via `nft -f', table_lookup() returns the table
that already exists in the kernel, so if you look up for objects that
are defined in the file, nft bails out with 'Set does not exist'.

Use table_lookup_global() function returns the existing table that is
defined in the file and that it is set as context via
ctx->handle->table.

This is not a complete fix, we should splice the existing kernel objects
into the userspace declaration. We just need some way to identify what
objects are already in the kernel so we don't send them again (otherwise
we will hit EEXIST errors). I'll follow up with this full fix asap.

Anyway, this patch fixes this shell test:

I: [OK]         ./testcases/sets/cache_handling_0

So at least by now we have all shell test returning OK. I'll add more
tests to catch the case I describe above once it is fixed too.

Cc: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 45d585d..1cd77cb 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -155,6 +155,20 @@ static int byteorder_conversion(struct eval_ctx *ctx, struct expr **expr,
 	return 0;
 }
 
+static struct table *table_lookup_global(struct eval_ctx *ctx)
+{
+	struct table *table;
+
+	if (ctx->table != NULL)
+		return ctx->cmd->table;
+
+	table = table_lookup(&ctx->cmd->handle);
+	if (table == NULL)
+		return NULL;
+
+	return table;
+}
+
 /*
  * Symbol expression: parse symbol and evaluate resulting expression.
  */
@@ -189,7 +203,7 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 		if (ret < 0)
 			return cmd_error(ctx, "Could not process rule: Cannot list sets");
 
-		table = table_lookup(&ctx->cmd->handle);
+		table = table_lookup_global(ctx);
 		if (table == NULL)
 			return cmd_error(ctx, "Could not process rule: Table '%s' does not exist",
 					 ctx->cmd->handle.table);
@@ -2073,20 +2087,6 @@ int stmt_evaluate(struct eval_ctx *ctx, struct stmt *stmt)
 	}
 }
 
-static struct table *table_lookup_global(struct eval_ctx *ctx)
-{
-	struct table *table;
-
-	if (ctx->table != NULL)
-		return ctx->cmd->table;
-
-	table = table_lookup(&ctx->cmd->handle);
-	if (table == NULL)
-		return NULL;
-
-	return table;
-}
-
 static int setelem_evaluate(struct eval_ctx *ctx, struct expr **expr)
 {
 	struct table *table;
-- 
2.1.4

Re: [PATCH 1/2 nft] src: revisit cache population logic

[Pablo Neira Ayuso]

I'm going to push this into the repo, I would like this becomes part
of the upcoming nft 0.6 release. Please let me know if you find any
problem with it.

Thanks.