iptables-restore allows to insert rules at a certain position which is
problematic for iptables-nft to realize since rule position is not
determined by number but handle of previous or following rule.
To fix this, make use of the rule cache (which holds in-kernel rules
with their associated handle already). Insert new rules at the right
position into that cache, then at commit time (i.e., in nft_action())
traverse each chain's rule list for new rules to add:
* New rules at beginning of list are inserted in reverse order without
reference of another rule, so that each consecutive rule is added
before all previous ones.
* Each time an "old" (i.e., in-kernel) rule is encountered, its handle
is stored (overwriting any previous ones').
* New rules following an old one are appended to the old one (by
specifying its handle) in reverse order, so that each consecutive rule
is inserted between the old one and previously appended ones.
For this to function, built-in chains need to be in cache. Consequently,
batch jobs with type NFT_COMPAT_CHAIN_ADD must not delete their payload.
A nice side-effect of that is when initializing builtin tables/chains,
no explicit call to nft_commit() is required anymore.
Signed-off-by: Phil Sutter <phil@xxxxxx>
---
iptables/nft.c | 182 +++++++++++-------
.../ipt-restore/0003-restore-ordering_0 | 94 +++++++++
.../testcases/iptables/0005-rule-replace_0 | 38 ++++
3 files changed, 243 insertions(+), 71 deletions(-)
create mode 100755 iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
create mode 100755 iptables/tests/shell/testcases/iptables/0005-rule-replace_0
diff --git a/iptables/nft.c b/iptables/nft.c
index 1ce1ecdd276be..ab1a361905287 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -644,6 +644,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
return;
batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
+ nftnl_chain_list_add_tail(c, h->table[table->type].chain_cache);
}
/* find if built-in table already exists */
@@ -1189,7 +1190,6 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
{
struct nftnl_chain *c;
struct nftnl_rule *r;
- int type;
/* If built-in chains don't exist for this table, create them */
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -1203,21 +1203,21 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
if (handle > 0) {
nftnl_rule_set(r, NFTNL_RULE_HANDLE, &handle);
- type = NFT_COMPAT_RULE_REPLACE;
- } else
- type = NFT_COMPAT_RULE_APPEND;
-
- if (batch_rule_add(h, type, r) < 0) {
- nftnl_rule_free(r);
- return 0;
+ if (batch_rule_add(h, NFT_COMPAT_RULE_REPLACE, r) < 0) {
+ nftnl_rule_free(r);
+ return 0;
+ }
}
if (verbose)
h->ops->print_rule(r, 0, FMT_PRINT_RULE);
c = nft_chain_find(h, table, chain);
- if (c)
- nftnl_chain_rule_add_tail(r, c);
+ if (!c) {
+ errno = ENOENT;
+ return 0;
+ }
+ nftnl_chain_rule_add_tail(r, c);
return 1;
}
@@ -2050,37 +2050,11 @@ int nft_rule_delete(struct nft_handle *h, const char *chain,
return ret;
}
-static struct nftnl_rule *
-nft_rule_add(struct nft_handle *h, const char *chain,
- const char *table, struct iptables_command_state *cs,
- uint64_t handle, bool verbose)
-{
- struct nftnl_rule *r;
-
- r = nft_rule_new(h, chain, table, cs);
- if (r == NULL)
- return NULL;
-
- if (handle > 0)
- nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, handle);
-
- if (batch_rule_add(h, NFT_COMPAT_RULE_INSERT, r) < 0) {
- nftnl_rule_free(r);
- return NULL;
- }
-
- if (verbose)
- h->ops->print_rule(r, 0, FMT_PRINT_RULE);
-
- return r;
-}
-
int nft_rule_insert(struct nft_handle *h, const char *chain,
const char *table, void *data, int rulenum, bool verbose)
{
- struct nftnl_rule *r, *new_rule;
+ struct nftnl_rule *r = NULL, *new_rule;
struct nftnl_chain *c;
- uint64_t handle = 0;
/* If built-in chains don't exist for this table, create them */
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2095,31 +2069,23 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
}
if (rulenum > 0) {
- r = nft_rule_find(h, c, data, rulenum);
+ r = nft_rule_find(h, c, data, rulenum - 1);
if (r == NULL) {
- /* special case: iptables allows to insert into
- * rule_count + 1 position.
- */
- r = nft_rule_find(h, c, data, rulenum - 1);
- if (r != NULL)
- return nft_rule_append(h, chain, table, data,
- 0, verbose);
-
errno = ENOENT;
goto err;
}
-
- handle = nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE);
- DEBUGP("adding after rule handle %"PRIu64"\n", handle);
}
- new_rule = nft_rule_add(h, chain, table, data, handle, verbose);
+ new_rule = nft_rule_new(h, chain, table, data);
if (!new_rule)
goto err;
- if (handle)
+ if (verbose)
+ h->ops->print_rule(new_rule, 0, FMT_PRINT_RULE);
+
+ if (r) /* insert new rule after r */
nftnl_chain_rule_insert_at(new_rule, r);
- else
+ else /* insert new rule at beginning of list */
nftnl_chain_rule_add(new_rule, c);
return 1;
@@ -2280,16 +2246,8 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
bool found = false;
/* If built-in chains don't exist for this table, create them */
- if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0) {
+ if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
nft_xt_builtin_init(h, table);
- /* Force table and chain creation, otherwise first iptables -L
- * lists no table/chains.
- */
- if (!list_empty(&h->obj_list)) {
- nft_commit(h);
- flush_chain_cache(h, NULL);
- }
- }
ops = nft_family_ops_lookup(h->family);
@@ -2395,16 +2353,8 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
int ret = 0;
/* If built-in chains don't exist for this table, create them */
- if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0) {
+ if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
nft_xt_builtin_init(h, table);
- /* Force table and chain creation, otherwise first iptables -L
- * lists no table/chains.
- */
- if (!list_empty(&h->obj_list)) {
- nft_commit(h);
- flush_chain_cache(h, NULL);
- }
- }
if (!nft_is_table_compatible(h, table)) {
xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use 'nft' tool.\n", table);
@@ -2523,8 +2473,8 @@ static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
break;
case NFT_COMPAT_CHAIN_ZERO:
case NFT_COMPAT_CHAIN_USER_ADD:
- break;
case NFT_COMPAT_CHAIN_ADD:
+ break;
case NFT_COMPAT_CHAIN_USER_DEL:
case NFT_COMPAT_CHAIN_USER_FLUSH:
case NFT_COMPAT_CHAIN_UPDATE:
@@ -2545,6 +2495,78 @@ static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
free(o);
}
+struct rule_stack_data {
+ struct nft_handle *h;
+ uint64_t handle;
+ uint32_t seq;
+};
+
+static int rule_stack_cb(struct nftnl_rule *r, void *data)
+{
+ struct rule_stack_data *rsd = data;
+ uint16_t flags = NLM_F_CREATE;
+
+ if (rsd->handle) {
+ /* append rule to previous in-kernel one */
+ flags |= NLM_F_APPEND;
+ nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, rsd->handle);
+ }
+ nft_compat_rule_batch_add(rsd->h, NFT_MSG_NEWRULE,
+ flags, rsd->seq++, r);
+ mnl_nft_batch_continue(rsd->h->batch);
+ nftnl_rule_list_del(r);
+ nftnl_rule_free(r);
+ return 0;
+}
+
+struct batch_from_chain_cache_data {
+ struct nft_handle *handle;
+ uint32_t seq;
+};
+
+static int batch_from_chain_cache(struct nftnl_chain *c, void *data)
+{
+ struct batch_from_chain_cache_data *d = data;
+ struct rule_stack_data rsd = {
+ .h = d->handle,
+ .seq = d->seq,
+ };
+ struct nftnl_rule_list *stack;
+ struct nftnl_rule_iter *iter;
+ struct nftnl_rule *r;
+
+ stack = nftnl_rule_list_alloc();
+ if (!stack)
+ return -1;
+
+ iter = nftnl_rule_iter_create(c);
+ if (!iter)
+ return -1;
+
+ r = nftnl_rule_iter_next(iter);
+ while (r) {
+ uint64_t handle = nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE);
+
+ if (!handle) {
+ /* new rule to be added, put onto stack */
+ nftnl_rule_list_del(r);
+ nftnl_rule_list_add(r, stack);
+ } else {
+ /* rule in kernel already, handle rules in stack */
+ nftnl_rule_list_foreach(stack, rule_stack_cb, &rsd);
+ d->seq = rsd.seq;
+ rsd.handle = handle;
+ }
+ r = nftnl_rule_iter_next(iter);
+ }
+ nftnl_rule_iter_destroy(iter);
+
+ nftnl_rule_list_foreach(stack, rule_stack_cb, &rsd);
+ nftnl_rule_list_free(stack);
+ d->seq = rsd.seq;
+ return 0;
+}
+
static int nft_action(struct nft_handle *h, int action)
{
struct obj_update *n, *tmp;
@@ -2628,6 +2650,24 @@ static int nft_action(struct nft_handle *h, int action)
mnl_nft_batch_continue(h->batch);
}
+ if (h->have_cache) {
+ struct batch_from_chain_cache_data d = {
+ .handle = h,
+ .seq = seq,
+ };
+
+ for (i = 0; i < NFT_TABLE_MAX; i++) {
+ const char *cur_table = h->tables[i].name;
+
+ if (!cur_table)
+ continue;
+
+ nftnl_chain_list_foreach(h->table[i].chain_cache,
+ batch_from_chain_cache, &d);
+ }
+ seq = d.seq;
+ }
+
switch (action) {
case NFT_COMPAT_COMMIT:
mnl_batch_end(h->batch, seq++);
diff --git a/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0 b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
new file mode 100755
index 0000000000000..44ee796ef44df
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
@@ -0,0 +1,94 @@
+#!/bin/bash
+
+# Make sure iptables-restore does the right thing
+# when encountering INSERT rules with index.
+
+set -e
+
+# show rules, drop uninteresting policy settings
+ipt_show() {
+ $XT_MULTI iptables -S | grep -v '^-P'
+}
+
+# basic issue reproducer
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -m comment --comment "appended rule" -j ACCEPT
+-I FORWARD 1 -m comment --comment "rule 1" -j ACCEPT
+-I FORWARD 2 -m comment --comment "rule 2" -j ACCEPT
+-I FORWARD 3 -m comment --comment "rule 3" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# insert rules into existing ruleset
+
+$XT_MULTI iptables-restore --noflush <<EOF
+*filter
+-I FORWARD 1 -m comment --comment "rule 0.5" -j ACCEPT
+-I FORWARD 3 -m comment --comment "rule 1.5" -j ACCEPT
+-I FORWARD 5 -m comment --comment "rule 2.5" -j ACCEPT
+-I FORWARD 7 -m comment --comment "rule 3.5" -j ACCEPT
+-I FORWARD 9 -m comment --comment "appended rule 2" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 0.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 1.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3.5" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 2" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# insert rules in between added ones
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -m comment --comment "appended rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 3" -j ACCEPT
+-I FORWARD 1 -m comment --comment "rule 1" -j ACCEPT
+-I FORWARD 3 -m comment --comment "rule 2" -j ACCEPT
+-I FORWARD 5 -m comment --comment "rule 3" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# test rule deletion in dump files
+
+$XT_MULTI iptables-restore --noflush <<EOF
+*filter
+-D FORWARD -m comment --comment "appended rule 1" -j ACCEPT
+-D FORWARD 3
+-I FORWARD 3 -m comment --comment "manually replaced rule 2" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "manually replaced rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
diff --git a/iptables/tests/shell/testcases/iptables/0005-rule-replace_0 b/iptables/tests/shell/testcases/iptables/0005-rule-replace_0
new file mode 100755
index 0000000000000..5a3e922e50672
--- /dev/null
+++ b/iptables/tests/shell/testcases/iptables/0005-rule-replace_0
@@ -0,0 +1,38 @@
+#!/bin/bash
+
+# test rule replacement
+
+set -e
+
+# show rules, drop uninteresting policy settings
+ipt_show() {
+ $XT_MULTI iptables -S | grep -v '^-P'
+}
+
+$XT_MULTI iptables -A FORWARD -m comment --comment "rule 1" -j ACCEPT
+$XT_MULTI iptables -A FORWARD -m comment --comment "rule 2" -j ACCEPT
+$XT_MULTI iptables -A FORWARD -m comment --comment "rule 3" -j ACCEPT
+
+$XT_MULTI iptables -R FORWARD 2 -m comment --comment "replaced 2" -j ACCEPT
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+$XT_MULTI iptables -R FORWARD 1 -m comment --comment "replaced 1" -j ACCEPT
+
+EXPECT='-A FORWARD -m comment --comment "replaced 1" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+$XT_MULTI iptables -R FORWARD 3 -m comment --comment "replaced 3" -j ACCEPT
+
+EXPECT='-A FORWARD -m comment --comment "replaced 1" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 2" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
--
2.19.0
