* [PATCH nft] evaluate: kill anon sets with one element
@ 2019-05-19 17:18 Florian Westphal
2019-05-24 19:21 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2019-05-19 17:18 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
convert "ip saddr { 1.1.1.1 }" to "ip saddr 1.1.1.1".
Both do the same, but second form is faster since no single-element
anon set is created.
Fix up the remaining test cases to expect transformations of the form
"meta l4proto { 33-55}" to "meta l4proto 33-55".
Signed-off-by: Florian Westphal <fw@strlen.de>
---
src/evaluate.c | 19 ++++++
tests/py/any/meta.t | 8 +--
tests/py/any/meta.t.json | 49 +++++++------
tests/py/any/meta.t.payload | 35 +++-------
tests/py/ip/igmp.t | 6 +-
tests/py/ip/igmp.t.json | 56 +++------------
tests/py/ip/igmp.t.payload | 76 +++------------------
tests/shell/testcases/nft-f/0016redefines_1 | 2 +-
8 files changed, 81 insertions(+), 170 deletions(-)
diff --git a/src/evaluate.c b/src/evaluate.c
index 21d9e146e587..cfdf48c6f5b7 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1253,6 +1253,7 @@ static int expr_evaluate_set_elem(struct eval_ctx *ctx, struct expr **expr)
static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
{
struct expr *set = *expr, *i, *next;
+ unsigned int count = 0;
list_for_each_entry_safe(i, next, &set->expressions, list) {
if (list_member_evaluate(ctx, &i) < 0)
@@ -1277,6 +1278,8 @@ static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
return expr_error(ctx->msgs, i,
"Set member is not constant");
+ count++;
+
if (i->etype == EXPR_SET) {
/* Merge recursive set definitions */
list_splice_tail_init(&i->expressions, &i->list);
@@ -1288,6 +1291,22 @@ static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
set->set_flags |= NFT_SET_INTERVAL;
}
+ if (!ctx->set && count == 1) {
+ i = list_first_entry(&set->expressions, struct expr, list);
+ if (i->etype == EXPR_SET_ELEM) {
+ switch (i->key->etype) {
+ case EXPR_RANGE:
+ case EXPR_VALUE:
+ *expr = i->key;
+ i->key = NULL;
+ expr_free(set);
+ return 0;
+ default:
+ break;
+ }
+ }
+ }
+
set->set_flags |= NFT_SET_CONSTANT;
set->dtype = ctx->ectx.dtype;
diff --git a/tests/py/any/meta.t b/tests/py/any/meta.t
index 3d3ddad94aa3..8aea664794ad 100644
--- a/tests/py/any/meta.t
+++ b/tests/py/any/meta.t
@@ -29,7 +29,7 @@ meta l4proto 33-45;ok
meta l4proto != 33-45;ok
meta l4proto { 33, 55, 67, 88};ok;meta l4proto { 33, 55, 67, 88}
meta l4proto != { 33, 55, 67, 88};ok
-meta l4proto != { 33-55};ok
+meta l4proto != { 33-55};ok;meta l4proto != 33-55
meta priority root;ok
meta priority none;ok
@@ -78,7 +78,7 @@ meta iiftype ppp;ok
meta oif "lo" accept;ok;oif "lo" accept
meta oif != "lo" accept;ok;oif != "lo" accept
-meta oif {"lo"} accept;ok;oif {"lo"} accept
+meta oif {"lo"} accept;ok;oif "lo" accept
meta oifname "dummy0";ok;oifname "dummy0"
meta oifname != "dummy0";ok;oifname != "dummy0"
@@ -166,9 +166,9 @@ meta iifgroup != 0;ok;iifgroup != "default"
meta iifgroup "default";ok;iifgroup "default"
meta iifgroup != "default";ok;iifgroup != "default"
meta iifgroup { 11,33};ok;iifgroup { 11,33}
-meta iifgroup {11-33};ok;iifgroup {11-33}
+meta iifgroup {11-33};ok;iifgroup 11-33
meta iifgroup != { 11,33};ok;iifgroup != { 11,33}
-meta iifgroup != {11-33};ok;iifgroup != {11-33}
+meta iifgroup != {11-33};ok;iifgroup != 11-33
meta oifgroup 0;ok;oifgroup "default"
meta oifgroup != 0;ok;oifgroup != "default"
meta oifgroup "default";ok;oifgroup "default"
diff --git a/tests/py/any/meta.t.json b/tests/py/any/meta.t.json
index e4d22fa749b5..b5fc767242fb 100644
--- a/tests/py/any/meta.t.json
+++ b/tests/py/any/meta.t.json
@@ -310,12 +310,15 @@
{
"match": {
"left": {
- "meta": { "key": "l4proto" }
+ "meta": {
+ "key": "l4proto"
+ }
},
"op": "!=",
"right": {
- "set": [
- { "range": [ 33, 55 ] }
+ "range": [
+ 33,
+ 55
]
}
}
@@ -934,14 +937,12 @@
{
"match": {
"left": {
- "meta": { "key": "oif" }
+ "meta": {
+ "key": "oif"
+ }
},
"op": "==",
- "right": {
- "set": [
- "lo"
- ]
- }
+ "right": "lo"
}
},
{
@@ -1889,33 +1890,38 @@
}
]
-# meta iifgroup {11-33}
+# meta iifgroup != { 11,33}
[
{
"match": {
"left": {
- "meta": { "key": "iifgroup" }
+ "meta": {
+ "key": "iifgroup"
+ }
},
- "op": "==",
+ "op": "!=",
"right": {
"set": [
- { "range": [ 11, 33 ] }
+ 11,
+ 33
]
}
}
}
]
-# meta iifgroup != { 11,33}
+# meta iifgroup {11-33}
[
{
"match": {
"left": {
- "meta": { "key": "iifgroup" }
+ "meta": {
+ "key": "iifgroup"
+ }
},
- "op": "!=",
+ "op": "==",
"right": {
- "set": [
+ "range": [
11,
33
]
@@ -1929,12 +1935,15 @@
{
"match": {
"left": {
- "meta": { "key": "iifgroup" }
+ "meta": {
+ "key": "iifgroup"
+ }
},
"op": "!=",
"right": {
- "set": [
- { "range": [ 11, 33 ] }
+ "range": [
+ 11,
+ 33
]
}
}
diff --git a/tests/py/any/meta.t.payload b/tests/py/any/meta.t.payload
index 2fb352430663..9733167cf07b 100644
--- a/tests/py/any/meta.t.payload
+++ b/tests/py/any/meta.t.payload
@@ -126,13 +126,10 @@ ip test-ip4 input
[ lookup reg 1 set __set%d 0x1 ]
# meta l4proto != { 33-55}
-__set%d test-ip4 7
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000021 : 0 [end] element 00000038 : 1 [end]
ip test-ip4 input
[ meta load l4proto => reg 1 ]
[ byteorder reg 1 = hton(reg 1, 2, 1) ]
- [ lookup reg 1 set __set%d 0x1 ]
+ [ range neq reg 1 0x00000021 0x00000037 ]
# meta mark 0x4
ip test-ip4 input
@@ -285,12 +282,9 @@ ip test-ip4 input
[ immediate reg 0 accept ]
# meta oif {"lo"} accept
-__set%d test-ip4 3
-__set%d test-ip4 0
- element 00000001 : 0 [end]
ip test-ip4 input
[ meta load oif => reg 1 ]
- [ lookup reg 1 set __set%d ]
+ [ cmp eq reg 1 0x00000001 ]
[ immediate reg 0 accept ]
# meta oifname "dummy0"
@@ -652,12 +646,9 @@ ip test-ip4 input
[ cmp neq reg 1 0x00000000 ]
# meta iifgroup {"default"}
-__set%d test-ip4 3
-__set%d test-ip4 0
- element 00000000 : 0 [end]
ip test-ip4 input
[ meta load iifgroup => reg 1 ]
- [ lookup reg 1 set __set%d ]
+ [ cmp eq reg 1 0x00000000 ]
# meta iifgroup { 11,33}
__set%d test-ip4 3
@@ -668,13 +659,11 @@ ip test-ip4 input
[ lookup reg 1 set __set%d ]
# meta iifgroup {11-33}
-__set%d test-ip4 7
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 0b000000 : 0 [end] element 22000000 : 1 [end]
ip test-ip4 input
[ meta load iifgroup => reg 1 ]
[ byteorder reg 1 = hton(reg 1, 4, 4) ]
- [ lookup reg 1 set __set%d ]
+ [ cmp gte reg 1 0x0b000000 ]
+ [ cmp lte reg 1 0x21000000 ]
# meta iifgroup != { 11,33}
__set%d test-ip4 3
@@ -685,13 +674,10 @@ ip test-ip4 input
[ lookup reg 1 set __set%d 0x1 ]
# meta iifgroup != {11-33}
-__set%d test-ip4 7
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 0b000000 : 0 [end] element 22000000 : 1 [end]
-ip test-ip4 input
+ip test-ip4 input
[ meta load iifgroup => reg 1 ]
[ byteorder reg 1 = hton(reg 1, 4, 4) ]
- [ lookup reg 1 set __set%d 0x1 ]
+ [ range neq reg 1 0x0b000000 0x21000000 ]
# meta oifgroup 0
ip test-ip4 input
@@ -714,12 +700,9 @@ ip test-ip4 input
[ cmp neq reg 1 0x00000000 ]
# meta oifgroup != {"default"}
-__set%d test-ip4 3
-__set%d test-ip4 0
- element 00000000 : 0 [end]
-ip test-ip4 input
+ip test-ip4 input
[ meta load oifgroup => reg 1 ]
- [ lookup reg 1 set __set%d 0x1 ]
+ [ cmp neq reg 1 0x00000000 ]
# meta oifgroup { 11,33}
__set%d test-ip4 3
diff --git a/tests/py/ip/igmp.t b/tests/py/ip/igmp.t
index 939dcc32b248..7a39348a7f84 100644
--- a/tests/py/ip/igmp.t
+++ b/tests/py/ip/igmp.t
@@ -16,10 +16,8 @@ igmp checksum 12343;ok
igmp checksum != 12343;ok
igmp checksum 11-343;ok
igmp checksum != 11-343;ok
-igmp checksum { 11-343};ok
-igmp checksum != { 11-343};ok
-igmp checksum { 1111, 222, 343};ok
-igmp checksum != { 1111, 222, 343};ok
+igmp checksum { 1111, 222, 343, 11-22 };ok
+igmp checksum != { 1111, 222, 343, 11-22 };ok
igmp mrt 10;ok
igmp mrt != 10;ok
diff --git a/tests/py/ip/igmp.t.json b/tests/py/ip/igmp.t.json
index 66dd3bb70c5b..722720166375 100644
--- a/tests/py/ip/igmp.t.json
+++ b/tests/py/ip/igmp.t.json
@@ -196,7 +196,7 @@
}
]
-# igmp checksum { 11-343}
+# igmp checksum { 1111, 222, 343, 11-22 }
[
{
"match": {
@@ -212,16 +212,19 @@
{
"range": [
11,
- 343
+ 22
]
- }
+ },
+ 222,
+ 343,
+ 1111
]
}
}
}
]
-# igmp checksum != { 11-343}
+# igmp checksum != { 1111, 222, 343, 11-22 }
[
{
"match": {
@@ -237,50 +240,9 @@
{
"range": [
11,
- 343
+ 22
]
- }
- ]
- }
- }
- }
-]
-
-# igmp checksum { 1111, 222, 343}
-[
- {
- "match": {
- "left": {
- "payload": {
- "field": "checksum",
- "protocol": "igmp"
- }
- },
- "op": "==",
- "right": {
- "set": [
- 222,
- 343,
- 1111
- ]
- }
- }
- }
-]
-
-# igmp checksum != { 1111, 222, 343}
-[
- {
- "match": {
- "left": {
- "payload": {
- "field": "checksum",
- "protocol": "igmp"
- }
- },
- "op": "!=",
- "right": {
- "set": [
+ },
222,
343,
1111
diff --git a/tests/py/ip/igmp.t.payload b/tests/py/ip/igmp.t.payload
index 1319c3246f0b..b34380650da5 100644
--- a/tests/py/ip/igmp.t.payload
+++ b/tests/py/ip/igmp.t.payload
@@ -62,26 +62,6 @@ ip test-ip4 input
[ payload load 2b @ transport header + 2 => reg 1 ]
[ range neq reg 1 0x00000b00 0x00005701 ]
-# igmp checksum { 11-343}
-__set%d test-ip4 7 size 3
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00005801 : 1 [end]
-ip test-ip4 input
- [ meta load l4proto => reg 1 ]
- [ cmp eq reg 1 0x00000002 ]
- [ payload load 2b @ transport header + 2 => reg 1 ]
- [ lookup reg 1 set __set%d ]
-
-# igmp checksum != { 11-343}
-__set%d test-ip4 7 size 3
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00005801 : 1 [end]
-ip test-ip4 input
- [ meta load l4proto => reg 1 ]
- [ cmp eq reg 1 0x00000002 ]
- [ payload load 2b @ transport header + 2 => reg 1 ]
- [ lookup reg 1 set __set%d 0x1 ]
-
# igmp checksum { 1111, 222, 343}
__set%d test-ip4 3 size 3
__set%d test-ip4 0
@@ -186,26 +166,6 @@ ip test-ip4 input
[ payload load 2b @ transport header + 2 => reg 1 ]
[ range neq reg 1 0x00000b00 0x00005701 ]
-# igmp checksum { 11-343}
-__set%d test-ip4 7 size 3
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00005801 : 1 [end]
-ip test-ip4 input
- [ meta load l4proto => reg 1 ]
- [ cmp eq reg 1 0x00000002 ]
- [ payload load 2b @ transport header + 2 => reg 1 ]
- [ lookup reg 1 set __set%d ]
-
-# igmp checksum != { 11-343}
-__set%d test-ip4 7 size 3
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00005801 : 1 [end]
-ip test-ip4 input
- [ meta load l4proto => reg 1 ]
- [ cmp eq reg 1 0x00000002 ]
- [ payload load 2b @ transport header + 2 => reg 1 ]
- [ lookup reg 1 set __set%d 0x1 ]
-
# igmp checksum { 1111, 222, 343}
__set%d test-ip4 3 size 3
__set%d test-ip4 0
@@ -310,41 +270,21 @@ ip test-ip4 input
[ payload load 2b @ transport header + 2 => reg 1 ]
[ range neq reg 1 0x00000b00 0x00005701 ]
-# igmp checksum { 11-343}
-__set%d test-ip4 7 size 3
+# igmp checksum { 1111, 222, 343, 11-22 }
+__set%d test-ip4 7 size 9
__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00005801 : 1 [end]
-ip test-ip4 input
+ element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00001700 : 1 [end] element 0000de00 : 0 [end] element 0000df00 : 1 [end] element 00005701 : 0 [end] element 00005801 : 1 [end] element 00005704 : 0 [end] element 00005804 : 1 [end]
+ip test-ip4 input
[ meta load l4proto => reg 1 ]
[ cmp eq reg 1 0x00000002 ]
[ payload load 2b @ transport header + 2 => reg 1 ]
[ lookup reg 1 set __set%d ]
-# igmp checksum != { 11-343}
-__set%d test-ip4 7 size 3
-__set%d test-ip4 0
- element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00005801 : 1 [end]
-ip test-ip4 input
- [ meta load l4proto => reg 1 ]
- [ cmp eq reg 1 0x00000002 ]
- [ payload load 2b @ transport header + 2 => reg 1 ]
- [ lookup reg 1 set __set%d 0x1 ]
-
-# igmp checksum { 1111, 222, 343}
-__set%d test-ip4 3 size 3
+# igmp checksum != { 1111, 222, 343, 11-22 }
+__set%d test-ip4 7 size 9
__set%d test-ip4 0
- element 00005704 : 0 [end] element 0000de00 : 0 [end] element 00005701 : 0 [end]
-ip test-ip4 input
- [ meta load l4proto => reg 1 ]
- [ cmp eq reg 1 0x00000002 ]
- [ payload load 2b @ transport header + 2 => reg 1 ]
- [ lookup reg 1 set __set%d ]
-
-# igmp checksum != { 1111, 222, 343}
-__set%d test-ip4 3 size 3
-__set%d test-ip4 0
- element 00005704 : 0 [end] element 0000de00 : 0 [end] element 00005701 : 0 [end]
-ip test-ip4 input
+ element 00000000 : 1 [end] element 00000b00 : 0 [end] element 00001700 : 1 [end] element 0000de00 : 0 [end] element 0000df00 : 1 [end] element 00005701 : 0 [end] element 00005801 : 1 [end] element 00005704 : 0 [end] element 00005804 : 1 [end]
+ip test-ip4 input
[ meta load l4proto => reg 1 ]
[ cmp eq reg 1 0x00000002 ]
[ payload load 2b @ transport header + 2 => reg 1 ]
diff --git a/tests/shell/testcases/nft-f/0016redefines_1 b/tests/shell/testcases/nft-f/0016redefines_1
index d0148d65da54..a6e857bd13c0 100755
--- a/tests/shell/testcases/nft-f/0016redefines_1
+++ b/tests/shell/testcases/nft-f/0016redefines_1
@@ -17,7 +17,7 @@ table ip x {
EXPECTED="table ip x {
chain y {
ip saddr { 1.1.1.1, 2.2.2.2 }
- ip saddr { 3.3.3.3 }
+ ip saddr 3.3.3.3
}
}"
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH nft] evaluate: kill anon sets with one element
2019-05-19 17:18 [PATCH nft] evaluate: kill anon sets with one element Florian Westphal
@ 2019-05-24 19:21 ` Pablo Neira Ayuso
2019-05-24 21:06 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-24 19:21 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Sun, May 19, 2019 at 07:18:38PM +0200, Florian Westphal wrote:
> convert "ip saddr { 1.1.1.1 }" to "ip saddr 1.1.1.1".
> Both do the same, but second form is faster since no single-element
> anon set is created.
>
> Fix up the remaining test cases to expect transformations of the form
> "meta l4proto { 33-55}" to "meta l4proto 33-55".
Last time we discussed this I think we agreed to spew a warning for
this to educate people on this.
My concern is: This is an optimization, are we going to do transparent
optimizations of the ruleset? I would like to explore at some point
automatic transformations for rulesets, also spot shadowed rules,
overlaps, and other sort of inconsistencies.
Are we going to do all that transparently?
Asking this because this is an optimization after all, and I'm not
sure I want to step in into making optimizations transparently. Even
if this one is fairly trivial.
I also don't like this path because we introduce one more assymmetry
between what the user adds a what the user fetches from the kernel.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nft] evaluate: kill anon sets with one element
2019-05-24 19:21 ` Pablo Neira Ayuso
@ 2019-05-24 21:06 ` Florian Westphal
2019-05-24 21:25 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2019-05-24 21:06 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, May 19, 2019 at 07:18:38PM +0200, Florian Westphal wrote:
> > convert "ip saddr { 1.1.1.1 }" to "ip saddr 1.1.1.1".
> > Both do the same, but second form is faster since no single-element
> > anon set is created.
> >
> > Fix up the remaining test cases to expect transformations of the form
> > "meta l4proto { 33-55}" to "meta l4proto 33-55".
>
> Last time we discussed this I think we agreed to spew a warning for
> this to educate people on this.
I decided against it.
Why adding a warning? We do not change what the rule does, and we do
not collapse different rules into one.
> My concern is: This is an optimization, are we going to do transparent
> optimizations of the ruleset? I would like to explore at some point
> automatic transformations for rulesets, also spot shadowed rules,
> overlaps, and other sort of inconsistencies.
>
> Are we going to do all that transparently?
I think it could be done on a case-by-case basis.
> Asking this because this is an optimization after all, and I'm not
> sure I want to step in into making optimizations transparently. Even
> if this one is fairly trivial.
>
> I also don't like this path because we introduce one more assymmetry
> between what the user adds a what the user fetches from the kernel.
nft add rule ip filter input ip protocol tcp tcp dport 22
-> tcp dport 22
nft add rule inet filter input tcp dport 22 tcp sport 55
-> tcp sport 55 tcp dport 22
I don't think there is any point in warning about this, so
why warn about tcp dport { 22 } ?
A warning should, IMO, be reserved for something where we
detect a actual problem, e.g.
tcp dport 22 tcp dport 55
and then we could print a 'will never match' warning for nft -f,
and maybe even refuse it for "nft add rule .."
Full ruleset optimization should, IMO, be done transparently as
well if we're confident such transformations are done correctly.
I would however only do it for nft -f, not for "nft add rule ..."
I see rulesets appearing that are very iptables like, e.g.
tcp dport 22 accept
tcp dport 80 accept
and so on, and I think it makes sense to allow nft to compact this.
We could add optimization level support to nft, i.e. only do basic
per-rule and do cross-rule optimizations only with a command line parameter.
I think a trivial one such as s/{ 22 }/22/ should just be done automatically.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nft] evaluate: kill anon sets with one element
2019-05-24 21:06 ` Florian Westphal
@ 2019-05-24 21:25 ` Pablo Neira Ayuso
2019-05-24 21:33 ` Pablo Neira Ayuso
2019-05-24 21:40 ` Florian Westphal
0 siblings, 2 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-24 21:25 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Fri, May 24, 2019 at 11:06:34PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, May 19, 2019 at 07:18:38PM +0200, Florian Westphal wrote:
> > > convert "ip saddr { 1.1.1.1 }" to "ip saddr 1.1.1.1".
> > > Both do the same, but second form is faster since no single-element
> > > anon set is created.
> > >
> > > Fix up the remaining test cases to expect transformations of the form
> > > "meta l4proto { 33-55}" to "meta l4proto 33-55".
> >
> > Last time we discussed this I think we agreed to spew a warning for
> > this to educate people on this.
>
> I decided against it.
> Why adding a warning? We do not change what the rule does, and we do
> not collapse different rules into one.
Yes, there is no semantic change.
Should we also do transparent transformation for
ct state { established,new }
I have seen rulesets like this too.
> > My concern is: This is an optimization, are we going to do transparent
> > optimizations of the ruleset? I would like to explore at some point
> > automatic transformations for rulesets, also spot shadowed rules,
> > overlaps, and other sort of inconsistencies.
> >
> > Are we going to do all that transparently?
>
> I think it could be done on a case-by-case basis.
OK.
> > Asking this because this is an optimization after all, and I'm not
> > sure I want to step in into making optimizations transparently. Even
> > if this one is fairly trivial.
> >
> > I also don't like this path because we introduce one more assymmetry
> > between what the user adds a what the user fetches from the kernel.
>
> nft add rule ip filter input ip protocol tcp tcp dport 22
> -> tcp dport 22
>
> nft add rule inet filter input tcp dport 22 tcp sport 55
> -> tcp sport 55 tcp dport 22
>
> I don't think there is any point in warning about this, so
> why warn about tcp dport { 22 } ?
>
> A warning should, IMO, be reserved for something where we
> detect a actual problem, e.g.
>
> tcp dport 22 tcp dport 55
>
> and then we could print a 'will never match' warning for nft -f,
> and maybe even refuse it for "nft add rule .."
>
> Full ruleset optimization should, IMO, be done transparently as
> well if we're confident such transformations are done correctly.
>
> I would however only do it for nft -f, not for "nft add rule ..."
Why only for nft -f and not for "nft add rule" ?
> I see rulesets appearing that are very iptables like, e.g.
>
> tcp dport 22 accept
> tcp dport 80 accept
>
> and so on, and I think it makes sense to allow nft to compact this.
Yes, we can do transformations for that case too.
> We could add optimization level support to nft, i.e. only do basic
> per-rule and do cross-rule optimizations only with a command line parameter.
We can add a new parameter to optimize rulesets, we can start with
something simple, ie.
* collapse consecutive several rules that come with the same
selectors, only values change.
* turn { 22 } into 22.
* turn ct state {new, established } into ct new,established.
> I think a trivial one such as s/{ 22 }/22/ should just be done automatically.
We could, yes. But I would prefer if we place all optimizations under
some new option and we document them.
I was expecting to have a discussion on this during the NFWS :-).
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nft] evaluate: kill anon sets with one element
2019-05-24 21:25 ` Pablo Neira Ayuso
@ 2019-05-24 21:33 ` Pablo Neira Ayuso
2019-05-24 21:40 ` Florian Westphal
1 sibling, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-24 21:33 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Fri, May 24, 2019 at 11:25:06PM +0200, Pablo Neira Ayuso wrote:
[...]
> We can add a new parameter to optimize rulesets, we can start with
> something simple, ie.
>
> * collapse consecutive several rules that come with the same
> selectors, only values change.
>
> * turn { 22 } into 22.
>
> * turn ct state {new, established } into ct new,established.
This new optimization option would work both for "nft add rule" and
"nft -f", and we can also include a mode that just prints the
optimization, similar to iptables-translate. So users can diff their
rulesets before and after.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH nft] evaluate: kill anon sets with one element
2019-05-24 21:25 ` Pablo Neira Ayuso
2019-05-24 21:33 ` Pablo Neira Ayuso
@ 2019-05-24 21:40 ` Florian Westphal
1 sibling, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2019-05-24 21:40 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, May 24, 2019 at 11:06:34PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Sun, May 19, 2019 at 07:18:38PM +0200, Florian Westphal wrote:
> > > > convert "ip saddr { 1.1.1.1 }" to "ip saddr 1.1.1.1".
> > > > Both do the same, but second form is faster since no single-element
> > > > anon set is created.
> > > >
> > > > Fix up the remaining test cases to expect transformations of the form
> > > > "meta l4proto { 33-55}" to "meta l4proto 33-55".
> > >
> > > Last time we discussed this I think we agreed to spew a warning for
> > > this to educate people on this.
> >
> > I decided against it.
> > Why adding a warning? We do not change what the rule does, and we do
> > not collapse different rules into one.
>
> Yes, there is no semantic change.
>
> Should we also do transparent transformation for
>
> ct state { established,new }
>
> I have seen rulesets like this too.
Hmm, to what should this be transformed?
ct state established,new ?
We can do it for this one, established and new can never be set at the
same time (or any other state for that matter).
[..]
> > Full ruleset optimization should, IMO, be done transparently as
> > well if we're confident such transformations are done correctly.
> >
> > I would however only do it for nft -f, not for "nft add rule ..."
>
> Why only for nft -f and not for "nft add rule" ?
I think if you do a 'nft add rule' then you expect to add a rule and
not to modify an existing one.
There is also the performance aspect, for 'nft add' you can just
build the rule object and pass that to kernel, but for optimizing
you need to fetch what is there and perform analysis on it.
I should have be more specific, with "nft -f" i meant a full rule
file, not a partial one (esp. one that has a 'flush ruleset'), or
something that at least contains a complete subset, e.g. a new
chain with n rules.
We could add a command line switch for this and allow optimizing in
any case, or add a new 'nft compact' or 'nft optimize' command that
would fetch the ruleset and print the 'optimized' version what nft
thinks can be coalesced/omitted etc.
What do you think?
> We can add a new parameter to optimize rulesets, we can start with
> something simple, ie.
>
> * collapse consecutive several rules that come with the same
> selectors, only values change.
>
> * turn { 22 } into 22.
>
> * turn ct state {new, established } into ct new,established.
>
> > I think a trivial one such as s/{ 22 }/22/ should just be done automatically.
>
> We could, yes. But I would prefer if we place all optimizations under
> some new option and we document them.
>
> I was expecting to have a discussion on this during the NFWS :-).
Sure, lets do that.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-24 21:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-19 17:18 [PATCH nft] evaluate: kill anon sets with one element Florian Westphal
2019-05-24 19:21 ` Pablo Neira Ayuso
2019-05-24 21:06 ` Florian Westphal
2019-05-24 21:25 ` Pablo Neira Ayuso
2019-05-24 21:33 ` Pablo Neira Ayuso
2019-05-24 21:40 ` Florian Westphal
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.