* [PATCH nft] tests: shell: check that rule add with index works with echo @ 2019-09-03 23:27 Eric Garver 2019-09-04 8:13 ` Phil Sutter 2019-09-05 15:54 ` Pablo Neira Ayuso 0 siblings, 2 replies; 7+ messages in thread From: Eric Garver @ 2019-09-03 23:27 UTC (permalink / raw) To: netfilter-devel; +Cc: Pablo Neira Ayuso If --echo is used the rule cache will not be populated. This causes rules added using the "index" keyword to be simply appended to the chain. The bug was introduced in commit 3ab02db5f836 ("cache: add NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). Signed-off-by: Eric Garver <eric@garver.life> --- I think the issue is in cache_evaluate(). It sets the flags to NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to fix it. So I'll start by submitting a test case. :) --- tests/shell/testcases/cache/0007_echo_cache_init_0 | 14 ++++++++++++++ .../cache/dumps/0007_echo_cache_init_0.nft | 7 +++++++ 2 files changed, 21 insertions(+) create mode 100755 tests/shell/testcases/cache/0007_echo_cache_init_0 create mode 100644 tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft diff --git a/tests/shell/testcases/cache/0007_echo_cache_init_0 b/tests/shell/testcases/cache/0007_echo_cache_init_0 new file mode 100755 index 000000000000..280a0d06bdc3 --- /dev/null +++ b/tests/shell/testcases/cache/0007_echo_cache_init_0 @@ -0,0 +1,14 @@ +#!/bin/bash + +set -e + +$NFT -i >/dev/null <<EOF +add table inet t +add chain inet t c +add rule inet t c accept comment "first" +add rule inet t c accept comment "third" +EOF + +# make sure the rule cache gets initialized when using echo option +# +$NFT --echo add rule inet t c index 0 accept comment '"second"' >/dev/null diff --git a/tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft b/tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft new file mode 100644 index 000000000000..c774ee72a683 --- /dev/null +++ b/tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft @@ -0,0 +1,7 @@ +table inet t { + chain c { + accept comment "first" + accept comment "second" + accept comment "third" + } +} -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH nft] tests: shell: check that rule add with index works with echo 2019-09-03 23:27 [PATCH nft] tests: shell: check that rule add with index works with echo Eric Garver @ 2019-09-04 8:13 ` Phil Sutter 2019-09-04 19:17 ` Pablo Neira Ayuso 2019-09-05 15:54 ` Pablo Neira Ayuso 1 sibling, 1 reply; 7+ messages in thread From: Phil Sutter @ 2019-09-04 8:13 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel, Eric Garver Pablo, On Tue, Sep 03, 2019 at 07:27:13PM -0400, Eric Garver wrote: > If --echo is used the rule cache will not be populated. This causes > rules added using the "index" keyword to be simply appended to the > chain. The bug was introduced in commit 3ab02db5f836 ("cache: add > NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). > > Signed-off-by: Eric Garver <eric@garver.life> > --- > I think the issue is in cache_evaluate(). It sets the flags to > NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to > fix it. So I'll start by submitting a test case. :) In 3ab02db5f836a ("cache: add NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"), you introduced NFT_CACHE_UPDATE to control whether rule_evaluate() should call rule_cache_update(), probably assuming the latter function merely changes cache depending on current command. In fact, this function also links rules if needed (see call to link_rules()). The old code you replaced also did not always call rule_cache_update(), but that was merely for sanity: If cache doesn't contain rules, there is no point in updating it with added/replaced/removed rules. The implicit logic is if we saw a rule command with 'index' reference, cache would be completed up to rule level (because of the necessary index to handle translation). I'm not sure why you introduced NFT_CACHE_UPDATE in the first place, but following my logic (and it seems to serve no other purpose) I would set that flag whenever NFT_CACHE_RULE_BIT gets set. So IMHO, NFT_CACHE_UPDATE is redundant. Cheers, Phil ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] tests: shell: check that rule add with index works with echo 2019-09-04 8:13 ` Phil Sutter @ 2019-09-04 19:17 ` Pablo Neira Ayuso 2019-09-04 19:31 ` Pablo Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2019-09-04 19:17 UTC (permalink / raw) To: Phil Sutter, netfilter-devel, Eric Garver On Wed, Sep 04, 2019 at 10:13:37AM +0200, Phil Sutter wrote: > Pablo, > > On Tue, Sep 03, 2019 at 07:27:13PM -0400, Eric Garver wrote: > > If --echo is used the rule cache will not be populated. This causes > > rules added using the "index" keyword to be simply appended to the > > chain. The bug was introduced in commit 3ab02db5f836 ("cache: add > > NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). > > > > Signed-off-by: Eric Garver <eric@garver.life> > > --- > > I think the issue is in cache_evaluate(). It sets the flags to > > NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to > > fix it. So I'll start by submitting a test case. :) > > In 3ab02db5f836a ("cache: add NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED > flags"), you introduced NFT_CACHE_UPDATE to control whether > rule_evaluate() should call rule_cache_update(), probably assuming the > latter function merely changes cache depending on current command. In > fact, this function also links rules if needed (see call to > link_rules()). > > The old code you replaced also did not always call rule_cache_update(), > but that was merely for sanity: If cache doesn't contain rules, there is > no point in updating it with added/replaced/removed rules. The implicit > logic is if we saw a rule command with 'index' reference, cache would be > completed up to rule level (because of the necessary index to handle > translation). > > I'm not sure why you introduced NFT_CACHE_UPDATE in the first place, but > following my logic (and it seems to serve no other purpose) I would set > that flag whenever NFT_CACHE_RULE_BIT gets set. So IMHO, > NFT_CACHE_UPDATE is redundant. Please, just go ahead simplify this in case you found a way to do it. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] tests: shell: check that rule add with index works with echo 2019-09-04 19:17 ` Pablo Neira Ayuso @ 2019-09-04 19:31 ` Pablo Neira Ayuso 0 siblings, 0 replies; 7+ messages in thread From: Pablo Neira Ayuso @ 2019-09-04 19:31 UTC (permalink / raw) To: Phil Sutter, netfilter-devel, Eric Garver On Wed, Sep 04, 2019 at 09:17:18PM +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 04, 2019 at 10:13:37AM +0200, Phil Sutter wrote: > > Pablo, > > > > On Tue, Sep 03, 2019 at 07:27:13PM -0400, Eric Garver wrote: > > > If --echo is used the rule cache will not be populated. This causes > > > rules added using the "index" keyword to be simply appended to the > > > chain. The bug was introduced in commit 3ab02db5f836 ("cache: add > > > NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). > > > > > > Signed-off-by: Eric Garver <eric@garver.life> > > > --- > > > I think the issue is in cache_evaluate(). It sets the flags to > > > NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to > > > fix it. So I'll start by submitting a test case. :) > > > > In 3ab02db5f836a ("cache: add NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED > > flags"), you introduced NFT_CACHE_UPDATE to control whether > > rule_evaluate() should call rule_cache_update(), probably assuming the > > latter function merely changes cache depending on current command. In > > fact, this function also links rules if needed (see call to > > link_rules()). > > > > The old code you replaced also did not always call rule_cache_update(), > > but that was merely for sanity: If cache doesn't contain rules, there is > > no point in updating it with added/replaced/removed rules. The implicit > > logic is if we saw a rule command with 'index' reference, cache would be > > completed up to rule level (because of the necessary index to handle > > translation). > > > > I'm not sure why you introduced NFT_CACHE_UPDATE in the first place, but > > following my logic (and it seems to serve no other purpose) I would set > > that flag whenever NFT_CACHE_RULE_BIT gets set. So IMHO, > > NFT_CACHE_UPDATE is redundant. > > Please, just go ahead simplify this in case you found a way to do it. IIRC, the idea was: * NFT_CACHE_UPDATE tells to update the cache incrementally. * NFT_CACHE_RULE tells to fetch the rule cache. I think you're right, they currently overlap, because if NFT_CACHE_RULE is requested, then NFT_CACHE_UPDATE necessarily needs to happen, right? Oh, there's one scenario where this is not the case: If flush is requested, then the NFT_CACHE_RULE flag is set off, while the NFT_CACHE_UPDATE is still left in place. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] tests: shell: check that rule add with index works with echo 2019-09-03 23:27 [PATCH nft] tests: shell: check that rule add with index works with echo Eric Garver 2019-09-04 8:13 ` Phil Sutter @ 2019-09-05 15:54 ` Pablo Neira Ayuso 2019-09-05 16:13 ` Eric Garver 1 sibling, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2019-09-05 15:54 UTC (permalink / raw) To: Eric Garver; +Cc: netfilter-devel On Tue, Sep 03, 2019 at 07:27:13PM -0400, Eric Garver wrote: > If --echo is used the rule cache will not be populated. This causes > rules added using the "index" keyword to be simply appended to the > chain. The bug was introduced in commit 3ab02db5f836 ("cache: add > NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). > > Signed-off-by: Eric Garver <eric@garver.life> > --- > I think the issue is in cache_evaluate(). It sets the flags to > NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to > fix it. So I'll start by submitting a test case. :) > --- > tests/shell/testcases/cache/0007_echo_cache_init_0 | 14 ++++++++++++++ > .../cache/dumps/0007_echo_cache_init_0.nft | 7 +++++++ > 2 files changed, 21 insertions(+) > create mode 100755 tests/shell/testcases/cache/0007_echo_cache_init_0 > create mode 100644 tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft > > diff --git a/tests/shell/testcases/cache/0007_echo_cache_init_0 b/tests/shell/testcases/cache/0007_echo_cache_init_0 > new file mode 100755 > index 000000000000..280a0d06bdc3 > --- /dev/null > +++ b/tests/shell/testcases/cache/0007_echo_cache_init_0 > @@ -0,0 +1,14 @@ > +#!/bin/bash > + > +set -e > + > +$NFT -i >/dev/null <<EOF > +add table inet t > +add chain inet t c > +add rule inet t c accept comment "first" > +add rule inet t c accept comment "third" > +EOF > + > +# make sure the rule cache gets initialized when using echo option > +# > +$NFT --echo add rule inet t c index 0 accept comment '"second"' >/dev/null Looks like the problem is index == 0? if (cmd->handle.index.id || cmd->handle.position.id) flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE; We might need to set up a flag, eg. cmd->handle.flags & NFT_HANDLE_INDEX, similarly with position, given that index.id can be zero. Alternatively, initialize index id to -1, assuming we'll never get to ~0U index. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] tests: shell: check that rule add with index works with echo 2019-09-05 15:54 ` Pablo Neira Ayuso @ 2019-09-05 16:13 ` Eric Garver 2019-09-05 17:25 ` Pablo Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Eric Garver @ 2019-09-05 16:13 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel On Thu, Sep 05, 2019 at 05:54:18PM +0200, Pablo Neira Ayuso wrote: > On Tue, Sep 03, 2019 at 07:27:13PM -0400, Eric Garver wrote: > > If --echo is used the rule cache will not be populated. This causes > > rules added using the "index" keyword to be simply appended to the > > chain. The bug was introduced in commit 3ab02db5f836 ("cache: add > > NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). > > > > Signed-off-by: Eric Garver <eric@garver.life> > > --- > > I think the issue is in cache_evaluate(). It sets the flags to > > NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to > > fix it. So I'll start by submitting a test case. :) > > --- > > tests/shell/testcases/cache/0007_echo_cache_init_0 | 14 ++++++++++++++ > > .../cache/dumps/0007_echo_cache_init_0.nft | 7 +++++++ > > 2 files changed, 21 insertions(+) > > create mode 100755 tests/shell/testcases/cache/0007_echo_cache_init_0 > > create mode 100644 tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft > > > > diff --git a/tests/shell/testcases/cache/0007_echo_cache_init_0 b/tests/shell/testcases/cache/0007_echo_cache_init_0 > > new file mode 100755 > > index 000000000000..280a0d06bdc3 > > --- /dev/null > > +++ b/tests/shell/testcases/cache/0007_echo_cache_init_0 > > @@ -0,0 +1,14 @@ > > +#!/bin/bash > > + > > +set -e > > + > > +$NFT -i >/dev/null <<EOF > > +add table inet t > > +add chain inet t c > > +add rule inet t c accept comment "first" > > +add rule inet t c accept comment "third" > > +EOF > > + > > +# make sure the rule cache gets initialized when using echo option > > +# > > +$NFT --echo add rule inet t c index 0 accept comment '"second"' >/dev/null > > Looks like the problem is index == 0? No. The index gets incremented by 1 by the JSON parser (CLI does the same thing). It's never zero if the "index" keyword is used. It's just as easily reproduced if you use any other index. > > if (cmd->handle.index.id || > cmd->handle.position.id) > flags |= NFT_CACHE_RULE | NFT_CACHE_UPDATE; We never reach this code since cache_evaluate() breaks if --echo is used. i.e. evaluate_cache_add() is never called. > We might need to set up a flag, eg. cmd->handle.flags & > NFT_HANDLE_INDEX, similarly with position, given that index.id can be > zero. Alternatively, initialize index id to -1, assuming we'll never > get to ~0U index. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nft] tests: shell: check that rule add with index works with echo 2019-09-05 16:13 ` Eric Garver @ 2019-09-05 17:25 ` Pablo Neira Ayuso 0 siblings, 0 replies; 7+ messages in thread From: Pablo Neira Ayuso @ 2019-09-05 17:25 UTC (permalink / raw) To: Eric Garver, netfilter-devel On Thu, Sep 05, 2019 at 12:13:54PM -0400, Eric Garver wrote: > On Thu, Sep 05, 2019 at 05:54:18PM +0200, Pablo Neira Ayuso wrote: > > On Tue, Sep 03, 2019 at 07:27:13PM -0400, Eric Garver wrote: > > > If --echo is used the rule cache will not be populated. This causes > > > rules added using the "index" keyword to be simply appended to the > > > chain. The bug was introduced in commit 3ab02db5f836 ("cache: add > > > NFT_CACHE_UPDATE and NFT_CACHE_FLUSHED flags"). > > > > > > Signed-off-by: Eric Garver <eric@garver.life> > > > --- > > > I think the issue is in cache_evaluate(). It sets the flags to > > > NFT_CACHE_FULL and then bails early, but I'm not sure of the best way to > > > fix it. So I'll start by submitting a test case. :) > > > --- > > > tests/shell/testcases/cache/0007_echo_cache_init_0 | 14 ++++++++++++++ > > > .../cache/dumps/0007_echo_cache_init_0.nft | 7 +++++++ > > > 2 files changed, 21 insertions(+) > > > create mode 100755 tests/shell/testcases/cache/0007_echo_cache_init_0 > > > create mode 100644 tests/shell/testcases/cache/dumps/0007_echo_cache_init_0.nft > > > > > > diff --git a/tests/shell/testcases/cache/0007_echo_cache_init_0 b/tests/shell/testcases/cache/0007_echo_cache_init_0 > > > new file mode 100755 > > > index 000000000000..280a0d06bdc3 > > > --- /dev/null > > > +++ b/tests/shell/testcases/cache/0007_echo_cache_init_0 > > > @@ -0,0 +1,14 @@ > > > +#!/bin/bash > > > + > > > +set -e > > > + > > > +$NFT -i >/dev/null <<EOF > > > +add table inet t > > > +add chain inet t c > > > +add rule inet t c accept comment "first" > > > +add rule inet t c accept comment "third" > > > +EOF > > > + > > > +# make sure the rule cache gets initialized when using echo option > > > +# > > > +$NFT --echo add rule inet t c index 0 accept comment '"second"' >/dev/null > > > > Looks like the problem is index == 0? > > No. The index gets incremented by 1 by the JSON parser (CLI does the > same thing). It's never zero if the "index" keyword is used. > > It's just as easily reproduced if you use any other index. I see, thanks. This one is passing tests here: https://patchwork.ozlabs.org/patch/1158616/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-05 17:25 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-03 23:27 [PATCH nft] tests: shell: check that rule add with index works with echo Eric Garver 2019-09-04 8:13 ` Phil Sutter 2019-09-04 19:17 ` Pablo Neira Ayuso 2019-09-04 19:31 ` Pablo Neira Ayuso 2019-09-05 15:54 ` Pablo Neira Ayuso 2019-09-05 16:13 ` Eric Garver 2019-09-05 17:25 ` Pablo Neira Ayuso
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.