src: revisit cache population logic

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>

diff --git a/src/evaluate.c b/src/evaluate.c
index b17cc82fd384c317f878c1480ff1c5788e16f413..be6ae593f797cb7908710780e64965d4c07f4165 100644 (file)
--- 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 ret;
+
                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 d643970f42a704b6338baee31c4ab0dc4916fc10..ad73d800de410cd7629871959e3e4e15d5f75bcf 100644 (file)
--- 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 42d60b4ab209d1f928d74efad2f641305556c110..855cceb85932be18d941a0dfa2018574620cefb4 100755 (executable)
--- 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 1bf688779dbfd0279ca2daa94f263bd5593fd9e4..75f2895ff7e50b74ae0e919f69804b4d279e0038 100755 (executable)
--- 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