Hi Phil,
On Sun, Dec 30, 2018 at 08:06:11PM +0100, Phil Sutter wrote:
> 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.
Looking at this late postprocessing in nft_action()... a bit of
brainstorming.
Why not same approach as in rule_translate_index() in nftables? I
guess answer is this is also incomplete, right?
If that is not enough, we could add support for NFTA_RULE_ID to
nf_tables_newrule(). This NFTA_RULE_ID attribute provides a way to
refer to rules coming in this batch. It is similar to NFTA_SET_ID,
however, currently it is only supported from the rule deletion path.
This ID is internal to the transaction - we can freely allocate IDs
for the new rules coming in the batch. My concern is that we may have
problems later on if we do not address limitations in the kernel
interface.
Or thinking if we can do this with the existing interface... see
below.
> 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.
I think this refers to rules like:
iptables -I chain 1 ...
This case we can just handle it by doing plain rule insertion.
> * 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 existing rules, we already have an absolute reference through the
handle, then the NLM_F_APPEND flag tells if we want to place it before
or after that rule.
What scenario is forcing us to do this late postprocessing below that
we cannot handle with the existing interface? :-)
Thanks!
[...]
> @@ -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
>
