netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).