* [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set
@ 2021-06-11 17:15 Pablo Neira Ayuso
2021-06-14 11:34 ` Phil Sutter
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-06-11 17:15 UTC (permalink / raw)
To: netfilter-devel
Perform the table and set lookup only for non-anonymous sets, where the
incremental cache update is required.
The problem fixed by 7aa08d45031e ("evaluate: Perform set evaluation on
implicitly declared (anonymous) sets") resurrected after the cache
rework.
# nft add rule x y tcp sport . tcp dport vmap { ssh . 0-65535 : accept, 0-65535 . ssh : accept }
BUG: invalid range expression type concat
nft: expression.c:1422: range_expr_value_low: Assertion `0' failed.
Abort
Add a test case to make sure this does not happen again.
Fixes: 5ec5c706d993 ("cache: add hashtable cache for table")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/evaluate.c | 17 +++++++++--------
tests/py/ip/ip.t | 2 ++
tests/py/ip/ip.t.payload | 9 +++++++++
tests/py/ip/ip.t.payload.bridge | 11 +++++++++++
tests/py/ip/ip.t.payload.inet | 11 +++++++++++
tests/py/ip/ip.t.payload.netdev | 11 +++++++++++
6 files changed, 53 insertions(+), 8 deletions(-)
diff --git a/src/evaluate.c b/src/evaluate.c
index 2ed68aad0392..e638046b33c7 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3781,15 +3781,16 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
struct stmt *stmt;
const char *type;
- table = table_cache_find(&ctx->nft->cache.table_cache,
- ctx->cmd->handle.table.name,
- ctx->cmd->handle.family);
- if (table == NULL)
- return table_not_found(ctx);
+ if (!(set->flags & NFT_SET_ANONYMOUS)) {
+ table = table_cache_find(&ctx->nft->cache.table_cache,
+ set->handle.table.name,
+ set->handle.family);
+ if (table == NULL)
+ return table_not_found(ctx);
- if (!(set->flags & NFT_SET_ANONYMOUS) &&
- !set_cache_find(table, set->handle.set.name))
- set_cache_add(set_get(set), table);
+ if (!set_cache_find(table, set->handle.set.name))
+ set_cache_add(set_get(set), table);
+ }
if (!(set->flags & NFT_SET_INTERVAL) && set->automerge)
return set_error(ctx, set, "auto-merge only works with interval sets");
diff --git a/tests/py/ip/ip.t b/tests/py/ip/ip.t
index 43c345cfa385..b74d465fcbe6 100644
--- a/tests/py/ip/ip.t
+++ b/tests/py/ip/ip.t
@@ -123,3 +123,5 @@ iif "lo" ip protocol set 1;ok
iif "lo" ip dscp set af23;ok
iif "lo" ip dscp set cs0;ok
+
+ip saddr . ip daddr { 192.0.2.1 . 10.0.0.1-10.0.0.2 };ok
diff --git a/tests/py/ip/ip.t.payload b/tests/py/ip/ip.t.payload
index 5ba7d6e963ac..4bb177526971 100644
--- a/tests/py/ip/ip.t.payload
+++ b/tests/py/ip/ip.t.payload
@@ -506,3 +506,12 @@ ip test-ip4 input
[ bitwise reg 1 = ( reg 1 & 0x000000ff ) ^ 0x00000100 ]
[ payload write reg 1 => 2b @ network header + 8 csum_type 1 csum_off 10 csum_flags 0x1 ]
+# ip saddr . ip daddr { 192.0.2.1 . 10.0.0.1-10.0.0.2 }
+__set%d test-ip4 87 size 1
+__set%d test-ip4 0
+ element 010200c0 0100000a - 010200c0 0200000a : 0 [end]
+ip
+ [ payload load 4b @ network header + 12 => reg 1 ]
+ [ payload load 4b @ network header + 16 => reg 9 ]
+ [ lookup reg 1 set __set%d ]
+
diff --git a/tests/py/ip/ip.t.payload.bridge b/tests/py/ip/ip.t.payload.bridge
index ead3156bc509..c8c1dbadee14 100644
--- a/tests/py/ip/ip.t.payload.bridge
+++ b/tests/py/ip/ip.t.payload.bridge
@@ -662,3 +662,14 @@ bridge test-bridge input
[ bitwise reg 1 = ( reg 1 & 0x000003ff ) ^ 0x00000000 ]
[ payload write reg 1 => 2b @ network header + 0 csum_type 1 csum_off 10 csum_flags 0x0 ]
+# ip saddr . ip daddr { 192.0.2.1 . 10.0.0.1-10.0.0.2 }
+__set%d test-bridge 87 size 1
+__set%d test-bridge 0
+ element 010200c0 0100000a - 010200c0 0200000a : 0 [end]
+bridge
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+ [ payload load 4b @ network header + 12 => reg 1 ]
+ [ payload load 4b @ network header + 16 => reg 9 ]
+ [ lookup reg 1 set __set%d ]
+
diff --git a/tests/py/ip/ip.t.payload.inet b/tests/py/ip/ip.t.payload.inet
index 0b08e0bf5756..55304fc9d871 100644
--- a/tests/py/ip/ip.t.payload.inet
+++ b/tests/py/ip/ip.t.payload.inet
@@ -662,3 +662,14 @@ inet test-inet input
[ bitwise reg 1 = ( reg 1 & 0x000000ff ) ^ 0x00000100 ]
[ payload write reg 1 => 2b @ network header + 8 csum_type 1 csum_off 10 csum_flags 0x1 ]
+# ip saddr . ip daddr { 192.0.2.1 . 10.0.0.1-10.0.0.2 }
+__set%d test-inet 87 size 1
+__set%d test-inet 0
+ element 010200c0 0100000a - 010200c0 0200000a : 0 [end]
+inet
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x00000002 ]
+ [ payload load 4b @ network header + 12 => reg 1 ]
+ [ payload load 4b @ network header + 16 => reg 9 ]
+ [ lookup reg 1 set __set%d ]
+
diff --git a/tests/py/ip/ip.t.payload.netdev b/tests/py/ip/ip.t.payload.netdev
index a4f56103d09a..712cb3756149 100644
--- a/tests/py/ip/ip.t.payload.netdev
+++ b/tests/py/ip/ip.t.payload.netdev
@@ -662,3 +662,14 @@ netdev test-netdev ingress
[ bitwise reg 1 = ( reg 1 & 0x000000ff ) ^ 0x00000100 ]
[ payload write reg 1 => 2b @ network header + 8 csum_type 1 csum_off 10 csum_flags 0x1 ]
+# ip saddr . ip daddr { 192.0.2.1 . 10.0.0.1-10.0.0.2 }
+__set%d test-netdev 87 size 1
+__set%d test-netdev 0
+ element 010200c0 0100000a - 010200c0 0200000a : 0 [end]
+netdev
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+ [ payload load 4b @ network header + 12 => reg 1 ]
+ [ payload load 4b @ network header + 16 => reg 9 ]
+ [ lookup reg 1 set __set%d ]
+
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set
2021-06-11 17:15 [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set Pablo Neira Ayuso
@ 2021-06-14 11:34 ` Phil Sutter
2021-06-14 11:56 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2021-06-14 11:34 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Hi Pablo,
On Fri, Jun 11, 2021 at 07:15:38PM +0200, Pablo Neira Ayuso wrote:
> Perform the table and set lookup only for non-anonymous sets, where the
> incremental cache update is required.
>
> The problem fixed by 7aa08d45031e ("evaluate: Perform set evaluation on
> implicitly declared (anonymous) sets") resurrected after the cache
> rework.
>
> # nft add rule x y tcp sport . tcp dport vmap { ssh . 0-65535 : accept, 0-65535 . ssh : accept }
> BUG: invalid range expression type concat
> nft: expression.c:1422: range_expr_value_low: Assertion `0' failed.
> Abort
>
> Add a test case to make sure this does not happen again.
>
> Fixes: 5ec5c706d993 ("cache: add hashtable cache for table")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
This triggers a warning:
evaluate.c: In function 'set_evaluate':
evaluate.c:3870:13: warning: 'table' may be used uninitialized in this function [-Wmaybe-uninitialized]
3870 | if (set_cache_find(table, set->handle.set.name) == NULL)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Cheers, Phil
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set
2021-06-14 11:34 ` Phil Sutter
@ 2021-06-14 11:56 ` Pablo Neira Ayuso
2021-06-14 11:58 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-06-14 11:56 UTC (permalink / raw)
To: Phil Sutter, netfilter-devel
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]
On Mon, Jun 14, 2021 at 01:34:03PM +0200, Phil Sutter wrote:
> Hi Pablo,
>
> On Fri, Jun 11, 2021 at 07:15:38PM +0200, Pablo Neira Ayuso wrote:
> > Perform the table and set lookup only for non-anonymous sets, where the
> > incremental cache update is required.
> >
> > The problem fixed by 7aa08d45031e ("evaluate: Perform set evaluation on
> > implicitly declared (anonymous) sets") resurrected after the cache
> > rework.
> >
> > # nft add rule x y tcp sport . tcp dport vmap { ssh . 0-65535 : accept, 0-65535 . ssh : accept }
> > BUG: invalid range expression type concat
> > nft: expression.c:1422: range_expr_value_low: Assertion `0' failed.
> > Abort
> >
> > Add a test case to make sure this does not happen again.
> >
> > Fixes: 5ec5c706d993 ("cache: add hashtable cache for table")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>
> This triggers a warning:
>
> evaluate.c: In function 'set_evaluate':
> evaluate.c:3870:13: warning: 'table' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 3870 | if (set_cache_find(table, set->handle.set.name) == NULL)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hm, this needs to be restricted to anonymous sets, attaching patch.
[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 444 bytes --]
diff --git a/src/evaluate.c b/src/evaluate.c
index 5311963a20c5..7cd90e2c1840 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3867,7 +3867,8 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
}
ctx->set = NULL;
- if (set_cache_find(table, set->handle.set.name) == NULL)
+ if (!(set->flags & NFT_SET_ANONYMOUS) &&
+ !set_cache_find(table, set->handle.set.name))
set_cache_add(set_get(set), table);
return 0;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set
2021-06-14 11:56 ` Pablo Neira Ayuso
@ 2021-06-14 11:58 ` Pablo Neira Ayuso
2021-06-14 12:04 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-06-14 11:58 UTC (permalink / raw)
To: Phil Sutter, netfilter-devel
On Mon, Jun 14, 2021 at 01:56:44PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 14, 2021 at 01:34:03PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> >
> > On Fri, Jun 11, 2021 at 07:15:38PM +0200, Pablo Neira Ayuso wrote:
> > > Perform the table and set lookup only for non-anonymous sets, where the
> > > incremental cache update is required.
> > >
> > > The problem fixed by 7aa08d45031e ("evaluate: Perform set evaluation on
> > > implicitly declared (anonymous) sets") resurrected after the cache
> > > rework.
> > >
> > > # nft add rule x y tcp sport . tcp dport vmap { ssh . 0-65535 : accept, 0-65535 . ssh : accept }
> > > BUG: invalid range expression type concat
> > > nft: expression.c:1422: range_expr_value_low: Assertion `0' failed.
> > > Abort
> > >
> > > Add a test case to make sure this does not happen again.
> > >
> > > Fixes: 5ec5c706d993 ("cache: add hashtable cache for table")
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >
> > This triggers a warning:
> >
> > evaluate.c: In function 'set_evaluate':
> > evaluate.c:3870:13: warning: 'table' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 3870 | if (set_cache_find(table, set->handle.set.name) == NULL)
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Hm, this needs to be restricted to anonymous sets, attaching patch.
It's a false positive though, there is a check for anonymous set a few
lines before. I'll apply this patch if this makes gcc happy on your side.
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 5311963a20c5..7cd90e2c1840 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3867,7 +3867,8 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
> }
> ctx->set = NULL;
>
> - if (set_cache_find(table, set->handle.set.name) == NULL)
> + if (!(set->flags & NFT_SET_ANONYMOUS) &&
> + !set_cache_find(table, set->handle.set.name))
> set_cache_add(set_get(set), table);
>
> return 0;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set
2021-06-14 11:58 ` Pablo Neira Ayuso
@ 2021-06-14 12:04 ` Pablo Neira Ayuso
0 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-06-14 12:04 UTC (permalink / raw)
To: Phil Sutter, netfilter-devel
[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]
On Mon, Jun 14, 2021 at 01:58:55PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 14, 2021 at 01:56:44PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jun 14, 2021 at 01:34:03PM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > >
> > > On Fri, Jun 11, 2021 at 07:15:38PM +0200, Pablo Neira Ayuso wrote:
> > > > Perform the table and set lookup only for non-anonymous sets, where the
> > > > incremental cache update is required.
> > > >
> > > > The problem fixed by 7aa08d45031e ("evaluate: Perform set evaluation on
> > > > implicitly declared (anonymous) sets") resurrected after the cache
> > > > rework.
> > > >
> > > > # nft add rule x y tcp sport . tcp dport vmap { ssh . 0-65535 : accept, 0-65535 . ssh : accept }
> > > > BUG: invalid range expression type concat
> > > > nft: expression.c:1422: range_expr_value_low: Assertion `0' failed.
> > > > Abort
> > > >
> > > > Add a test case to make sure this does not happen again.
> > > >
> > > > Fixes: 5ec5c706d993 ("cache: add hashtable cache for table")
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > >
> > > This triggers a warning:
> > >
> > > evaluate.c: In function 'set_evaluate':
> > > evaluate.c:3870:13: warning: 'table' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > > 3870 | if (set_cache_find(table, set->handle.set.name) == NULL)
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Hm, this needs to be restricted to anonymous sets, attaching patch.
>
> It's a false positive though, there is a check for anonymous set a few
> lines before. I'll apply this patch if this makes gcc happy on your side.
Actually, this code should not be there. The set is already in the cache.
[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 354 bytes --]
diff --git a/src/evaluate.c b/src/evaluate.c
index 5311963a20c5..92cc8994c809 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3867,9 +3867,6 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
}
ctx->set = NULL;
- if (set_cache_find(table, set->handle.set.name) == NULL)
- set_cache_add(set_get(set), table);
-
return 0;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-14 12:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11 17:15 [PATCH nft 2/2] evaluate: restore interval + concatenation in anonymous set Pablo Neira Ayuso
2021-06-14 11:34 ` Phil Sutter
2021-06-14 11:56 ` Pablo Neira Ayuso
2021-06-14 11:58 ` Pablo Neira Ayuso
2021-06-14 12:04 ` 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.