* [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill
@ 2021-08-26 10:49 Pablo Neira Ayuso
2021-08-26 10:59 ` Florian Westphal
2021-08-30 16:40 ` Phil Sutter
0 siblings, 2 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-08-26 10:49 UTC (permalink / raw)
To: netfilter-devel
meta protocol is meaningful in bridge, netdev and inet familiiess, do
not remove this.
Fixes: a1bcf8a34975 ("payload: add payload_may_dependency_kill()")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/netlink_delinearize.c | 22 ++++++++++++--
tests/py/bridge/meta.t | 3 ++
tests/py/bridge/meta.t.json | 54 ++++++++++++++++++++++++++++++++++
tests/py/bridge/meta.t.payload | 18 ++++++++++++
tests/py/inet/meta.t | 4 +++
tests/py/inet/meta.t.json | 54 ++++++++++++++++++++++++++++++++++
tests/py/inet/meta.t.payload | 18 ++++++++++++
tests/py/ip/meta.t | 2 ++
tests/py/ip/meta.t.json | 16 ++++++++++
tests/py/ip/meta.t.payload | 9 ++++++
tests/py/ip6/meta.t | 3 ++
tests/py/ip6/meta.t.json | 54 ++++++++++++++++++++++++++++++++++
tests/py/ip6/meta.t.payload | 18 ++++++++++++
13 files changed, 272 insertions(+), 3 deletions(-)
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 5b545701e8b7..92617a46df6f 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1993,7 +1993,7 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
const struct expr *expr)
{
struct expr *dep = ctx->pdep->expr;
- uint16_t l3proto;
+ uint16_t l3proto, protocol;
uint8_t l4proto;
if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
@@ -2005,7 +2005,22 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
case NFPROTO_BRIDGE:
break;
default:
- return true;
+ if (dep->left->etype != EXPR_META ||
+ dep->right->etype != EXPR_VALUE)
+ return false;
+
+ if (dep->left->meta.key == NFT_META_PROTOCOL) {
+ protocol = mpz_get_uint16(dep->right->value);
+
+ if (family == NFPROTO_IPV4 &&
+ protocol == ETH_P_IP)
+ return true;
+ else if (family == NFPROTO_IPV6 &&
+ protocol == ETH_P_IPV6)
+ return true;
+ }
+
+ return false;
}
if (expr->left->meta.key != NFT_META_L4PROTO)
@@ -2015,7 +2030,8 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
switch (dep->left->etype) {
case EXPR_META:
- if (dep->left->meta.key != NFT_META_NFPROTO)
+ if (dep->left->meta.key != NFT_META_NFPROTO &&
+ dep->left->meta.key != NFT_META_PROTOCOL)
return true;
break;
case EXPR_PAYLOAD:
diff --git a/tests/py/bridge/meta.t b/tests/py/bridge/meta.t
index eda7082f02b4..d77ebd899f18 100644
--- a/tests/py/bridge/meta.t
+++ b/tests/py/bridge/meta.t
@@ -6,3 +6,6 @@ meta obrname "br0";ok
meta ibrname "br0";ok
meta ibrvproto vlan;ok;meta ibrvproto 8021q
meta ibrpvid 100;ok
+
+meta protocol ip udp dport 67;ok
+meta protocol ip6 udp dport 67;ok
diff --git a/tests/py/bridge/meta.t.json b/tests/py/bridge/meta.t.json
index 3122774eba8c..d7dc9d7b5ea7 100644
--- a/tests/py/bridge/meta.t.json
+++ b/tests/py/bridge/meta.t.json
@@ -49,3 +49,57 @@
}
}
]
+
+# meta protocol ip udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "meta": {
+ "key": "protocol"
+ }
+ },
+ "op": "==",
+ "right": "ip"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
+
+# meta protocol ip6 udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "meta": {
+ "key": "protocol"
+ }
+ },
+ "op": "==",
+ "right": "ip6"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
diff --git a/tests/py/bridge/meta.t.payload b/tests/py/bridge/meta.t.payload
index aa8c994bfe58..14177767fdfc 100644
--- a/tests/py/bridge/meta.t.payload
+++ b/tests/py/bridge/meta.t.payload
@@ -17,3 +17,21 @@ bridge test-bridge input
bridge test-bridge input
[ meta load bri_iifpvid => reg 1 ]
[ cmp eq reg 1 0x00000064 ]
+
+# meta protocol ip udp dport 67
+bridge
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
+
+# meta protocol ip6 udp dport 67
+bridge
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x0000dd86 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
diff --git a/tests/py/inet/meta.t b/tests/py/inet/meta.t
index 3638898b5dbb..423cc5f32cba 100644
--- a/tests/py/inet/meta.t
+++ b/tests/py/inet/meta.t
@@ -12,6 +12,10 @@ meta nfproto ipv4 tcp dport 22;ok
meta nfproto ipv4 ip saddr 1.2.3.4;ok;ip saddr 1.2.3.4
meta nfproto ipv6 meta l4proto tcp;ok;meta nfproto ipv6 meta l4proto 6
meta nfproto ipv4 counter ip saddr 1.2.3.4;ok
+
+meta protocol ip udp dport 67;ok
+meta protocol ip6 udp dport 67;ok
+
meta ipsec exists;ok
meta secpath missing;ok;meta ipsec missing
meta ibrname "br0";fail
diff --git a/tests/py/inet/meta.t.json b/tests/py/inet/meta.t.json
index 5c0e7d2e0e42..723a36f74946 100644
--- a/tests/py/inet/meta.t.json
+++ b/tests/py/inet/meta.t.json
@@ -235,3 +235,57 @@
}
}
]
+
+# meta protocol ip udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "meta": {
+ "key": "protocol"
+ }
+ },
+ "op": "==",
+ "right": "ip"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
+
+# meta protocol ip6 udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "meta": {
+ "key": "protocol"
+ }
+ },
+ "op": "==",
+ "right": "ip6"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
diff --git a/tests/py/inet/meta.t.payload b/tests/py/inet/meta.t.payload
index 6ccf6d24210a..f2c861378c61 100644
--- a/tests/py/inet/meta.t.payload
+++ b/tests/py/inet/meta.t.payload
@@ -79,3 +79,21 @@ inet test-inet input
[ ct load mark => reg 1 ]
[ bitwise reg 1 = ( reg 1 >> 0x00000008 ) ]
[ meta set mark with reg 1 ]
+
+# meta protocol ip udp dport 67
+inet
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
+
+# meta protocol ip6 udp dport 67
+inet
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x0000dd86 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
diff --git a/tests/py/ip/meta.t b/tests/py/ip/meta.t
index f733d22de2c3..fecd0caf71a7 100644
--- a/tests/py/ip/meta.t
+++ b/tests/py/ip/meta.t
@@ -8,6 +8,8 @@ meta l4proto ipv6-icmp icmpv6 type nd-router-advert;ok;icmpv6 type nd-router-adv
meta l4proto 58 icmpv6 type nd-router-advert;ok;icmpv6 type nd-router-advert
icmpv6 type nd-router-advert;ok
+meta protocol ip udp dport 67;ok
+
meta ibrname "br0";fail
meta obrname "br0";fail
diff --git a/tests/py/ip/meta.t.json b/tests/py/ip/meta.t.json
index f83864f672d5..3df31ce381fc 100644
--- a/tests/py/ip/meta.t.json
+++ b/tests/py/ip/meta.t.json
@@ -140,3 +140,19 @@
"accept": null
}
]
+
+# meta protocol ip udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
diff --git a/tests/py/ip/meta.t.payload b/tests/py/ip/meta.t.payload
index 7bc69a290d24..ebff0e4bb3b0 100644
--- a/tests/py/ip/meta.t.payload
+++ b/tests/py/ip/meta.t.payload
@@ -44,3 +44,12 @@ ip6 test-ip4 input
[ meta load sdifname => reg 1 ]
[ cmp neq reg 1 0x31667276 0x00000000 0x00000000 0x00000000 ]
[ immediate reg 0 accept ]
+
+# meta protocol ip udp dport 67
+ip
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
diff --git a/tests/py/ip6/meta.t b/tests/py/ip6/meta.t
index dce97f5b0fd0..2c1aee2309a9 100644
--- a/tests/py/ip6/meta.t
+++ b/tests/py/ip6/meta.t
@@ -9,5 +9,8 @@ meta l4proto icmp icmp type echo-request;ok;icmp type echo-request
meta l4proto 1 icmp type echo-request;ok;icmp type echo-request
icmp type echo-request;ok
+meta protocol ip udp dport 67;ok
+meta protocol ip6 udp dport 67;ok
+
meta sdif "lo" accept;ok
meta sdifname != "vrf1" accept;ok
diff --git a/tests/py/ip6/meta.t.json b/tests/py/ip6/meta.t.json
index e72350f375e9..351320d70f7c 100644
--- a/tests/py/ip6/meta.t.json
+++ b/tests/py/ip6/meta.t.json
@@ -140,3 +140,57 @@
"accept": null
}
]
+
+# meta protocol ip udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "meta": {
+ "key": "protocol"
+ }
+ },
+ "op": "==",
+ "right": "ip"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
+
+# meta protocol ip6 udp dport 67
+[
+ {
+ "match": {
+ "left": {
+ "meta": {
+ "key": "protocol"
+ }
+ },
+ "op": "==",
+ "right": "ip6"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "dport",
+ "protocol": "udp"
+ }
+ },
+ "op": "==",
+ "right": 67
+ }
+ }
+]
diff --git a/tests/py/ip6/meta.t.payload b/tests/py/ip6/meta.t.payload
index be04816eeec2..d654b019e7a7 100644
--- a/tests/py/ip6/meta.t.payload
+++ b/tests/py/ip6/meta.t.payload
@@ -44,3 +44,21 @@ ip6 test-ip6 input
[ meta load sdifname => reg 1 ]
[ cmp neq reg 1 0x31667276 0x00000000 0x00000000 0x00000000 ]
[ immediate reg 0 accept ]
+
+# meta protocol ip udp dport 67
+ip6
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
+
+# meta protocol ip6 udp dport 67
+ip6
+ [ meta load protocol => reg 1 ]
+ [ cmp eq reg 1 0x0000dd86 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000011 ]
+ [ payload load 2b @ transport header + 2 => reg 1 ]
+ [ cmp eq reg 1 0x00004300 ]
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill
2021-08-26 10:49 [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill Pablo Neira Ayuso
@ 2021-08-26 10:59 ` Florian Westphal
2021-08-30 16:40 ` Phil Sutter
1 sibling, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2021-08-26 10:59 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> meta protocol is meaningful in bridge, netdev and inet familiiess, do
> not remove this.
>
> Fixes: a1bcf8a34975 ("payload: add payload_may_dependency_kill()")
Looks like its a regression from
"netlink_delinearize: Refactor meta_may_dependency_kill()",
056aaa3e6dc65aced5e552233ac3e7f89fb81f86.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill
2021-08-26 10:49 [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill Pablo Neira Ayuso
2021-08-26 10:59 ` Florian Westphal
@ 2021-08-30 16:40 ` Phil Sutter
2021-08-30 17:21 ` Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2021-08-30 16:40 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal
Hi,
On Thu, Aug 26, 2021 at 12:49:52PM +0200, Pablo Neira Ayuso wrote:
> meta protocol is meaningful in bridge, netdev and inet familiiess, do
> not remove this.
So you want to avoid dependency killing in above families, but:
[...]
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index 5b545701e8b7..92617a46df6f 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
[...]
> @@ -2005,7 +2005,22 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
> case NFPROTO_BRIDGE:
> break;
> default:
> - return true;
> + if (dep->left->etype != EXPR_META ||
> + dep->right->etype != EXPR_VALUE)
> + return false;
> +
> + if (dep->left->meta.key == NFT_META_PROTOCOL) {
> + protocol = mpz_get_uint16(dep->right->value);
> +
> + if (family == NFPROTO_IPV4 &&
> + protocol == ETH_P_IP)
> + return true;
> + else if (family == NFPROTO_IPV6 &&
> + protocol == ETH_P_IPV6)
> + return true;
> + }
> +
> + return false;
> }
>
> if (expr->left->meta.key != NFT_META_L4PROTO)
The above code only applies to families other than inet, netdev or
bridge?! Am I missing something?
AFAIU, dependency killing defaults to true and examines the cases where
it should not happen. So the old behaviour was for unexpected dependency
expressions to just drop them instead of keeping them. The code you add
above does the opposite. That's not wrong per se, but I assume there was
a reason why dependency elimination was implemented "greedy", therefore
I kept it that way when refactoring the code.
> @@ -2015,7 +2030,8 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
>
> switch (dep->left->etype) {
> case EXPR_META:
> - if (dep->left->meta.key != NFT_META_NFPROTO)
> + if (dep->left->meta.key != NFT_META_NFPROTO &&
> + dep->left->meta.key != NFT_META_PROTOCOL)
> return true;
> break;
> case EXPR_PAYLOAD:
If we continue evaluation for 'meta protocol' payload dependencies, RHS
values need adjustment: with 'meta nfproto' we'll see NFPROTO_IPV4 or
NFPROTO_IPV6 while with 'meta protocol' we'll see ETH_P_IP or
ETH_P_IPV6. The EXPR_PAYLOAD case has such conversion code.
[...]
> diff --git a/tests/py/ip/meta.t b/tests/py/ip/meta.t
> index f733d22de2c3..fecd0caf71a7 100644
> --- a/tests/py/ip/meta.t
> +++ b/tests/py/ip/meta.t
> @@ -8,6 +8,8 @@ meta l4proto ipv6-icmp icmpv6 type nd-router-advert;ok;icmpv6 type nd-router-adv
> meta l4proto 58 icmpv6 type nd-router-advert;ok;icmpv6 type nd-router-advert
> icmpv6 type nd-router-advert;ok
>
> +meta protocol ip udp dport 67;ok
> +
Hmm. Shouldn't this drop the dependency, i.e. list as 'udp dport 67' as
the family is defined by table family already?
There should also be a case for 'meta protocol ip6 udp dport 67' which
makes sure the dependency is not dropped, right?
> meta ibrname "br0";fail
> meta obrname "br0";fail
>
> diff --git a/tests/py/ip/meta.t.json b/tests/py/ip/meta.t.json
> index f83864f672d5..3df31ce381fc 100644
> --- a/tests/py/ip/meta.t.json
> +++ b/tests/py/ip/meta.t.json
> @@ -140,3 +140,19 @@
> "accept": null
> }
> ]
> +
> +# meta protocol ip udp dport 67
> +[
> + {
> + "match": {
> + "left": {
> + "payload": {
> + "field": "dport",
> + "protocol": "udp"
> + }
> + },
> + "op": "==",
> + "right": 67
> + }
> + }
> +]
This translation is not correct, the 'meta protocol' match is missing in
JSON. Even though it is not present when listing ruleset, the idea is
that JSON is identical to standard syntax input to avoid any "short
cuts" (I guess it's just a typo, other spots are fine).
[...]
> diff --git a/tests/py/ip6/meta.t b/tests/py/ip6/meta.t
> index dce97f5b0fd0..2c1aee2309a9 100644
> --- a/tests/py/ip6/meta.t
> +++ b/tests/py/ip6/meta.t
> @@ -9,5 +9,8 @@ meta l4proto icmp icmp type echo-request;ok;icmp type echo-request
> meta l4proto 1 icmp type echo-request;ok;icmp type echo-request
> icmp type echo-request;ok
>
> +meta protocol ip udp dport 67;ok
> +meta protocol ip6 udp dport 67;ok
> +
Here, 'meta protocol ip6' should be dropped.
Pablo, do you want to have another look at your patch or should I try to
fix things up?
Cheers, Phil
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill
2021-08-30 16:40 ` Phil Sutter
@ 2021-08-30 17:21 ` Pablo Neira Ayuso
2021-08-30 17:53 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-08-30 17:21 UTC (permalink / raw)
To: Phil Sutter, netfilter-devel, Florian Westphal
Hi Phil,
On Mon, Aug 30, 2021 at 06:40:41PM +0200, Phil Sutter wrote:
> Hi,
>
> On Thu, Aug 26, 2021 at 12:49:52PM +0200, Pablo Neira Ayuso wrote:
> > meta protocol is meaningful in bridge, netdev and inet familiiess, do
> > not remove this.
>
> So you want to avoid dependency killing in above families, but:
>
> [...]
> > diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> > index 5b545701e8b7..92617a46df6f 100644
> > --- a/src/netlink_delinearize.c
> > +++ b/src/netlink_delinearize.c
> [...]
> > @@ -2005,7 +2005,22 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
> > case NFPROTO_BRIDGE:
> > break;
> > default:
> > - return true;
> > + if (dep->left->etype != EXPR_META ||
> > + dep->right->etype != EXPR_VALUE)
> > + return false;
> > +
> > + if (dep->left->meta.key == NFT_META_PROTOCOL) {
> > + protocol = mpz_get_uint16(dep->right->value);
> > +
> > + if (family == NFPROTO_IPV4 &&
> > + protocol == ETH_P_IP)
> > + return true;
> > + else if (family == NFPROTO_IPV6 &&
> > + protocol == ETH_P_IPV6)
> > + return true;
> > + }
> > +
> > + return false;
> > }
> >
> > if (expr->left->meta.key != NFT_META_L4PROTO)
>
> The above code only applies to families other than inet, netdev or
> bridge?! Am I missing something?
Yes, this code only applies to ip and ip6, this removes "meta
protocol" in this cases:
rule ip x y meta protocol ip udp dport 67 => udp dport 67
rule ip6 x y meta protocol ip6 udp dport 67 => udp dport 67
I should have keep it in a separated patch actually since it is not
related to bridge/inet/netdev. Sorry for the noise.
> AFAIU, dependency killing defaults to true and examines the cases where
> it should not happen. So the old behaviour was for unexpected dependency
> expressions to just drop them instead of keeping them. The code you add
> above does the opposite. That's not wrong per se, but I assume there was
> a reason why dependency elimination was implemented "greedy", therefore
> I kept it that way when refactoring the code.
>
> > @@ -2015,7 +2030,8 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
> >
> > switch (dep->left->etype) {
> > case EXPR_META:
> > - if (dep->left->meta.key != NFT_META_NFPROTO)
> > + if (dep->left->meta.key != NFT_META_NFPROTO &&
> > + dep->left->meta.key != NFT_META_PROTOCOL)
> > return true;
> > break;
> > case EXPR_PAYLOAD:
>
> If we continue evaluation for 'meta protocol' payload dependencies, RHS
> values need adjustment: with 'meta nfproto' we'll see NFPROTO_IPV4 or
> NFPROTO_IPV6 while with 'meta protocol' we'll see ETH_P_IP or
> ETH_P_IPV6. The EXPR_PAYLOAD case has such conversion code.
The chunk above the actual fix for the reported issue.
Yes, checking for ETH_P_IP and ETH_P_IPV6 would be good to narrow down
the dependency killing more precisely.
> [...]
> > diff --git a/tests/py/ip/meta.t b/tests/py/ip/meta.t
> > index f733d22de2c3..fecd0caf71a7 100644
> > --- a/tests/py/ip/meta.t
> > +++ b/tests/py/ip/meta.t
> > @@ -8,6 +8,8 @@ meta l4proto ipv6-icmp icmpv6 type nd-router-advert;ok;icmpv6 type nd-router-adv
> > meta l4proto 58 icmpv6 type nd-router-advert;ok;icmpv6 type nd-router-advert
> > icmpv6 type nd-router-advert;ok
> >
> > +meta protocol ip udp dport 67;ok
> > +
>
> Hmm. Shouldn't this drop the dependency, i.e. list as 'udp dport 67' as
> the family is defined by table family already?
It does actually, this spews a warning if you run tests/py.
> There should also be a case for 'meta protocol ip6 udp dport 67' which
> makes sure the dependency is not dropped, right?
Yes, I forgot to add it to ip/meta.t
I added it to ip6/meta.t
--- a/tests/py/ip6/meta.t
+++ b/tests/py/ip6/meta.t
@@ -9,5 +9,8 @@ meta l4proto icmp icmp type echo-request;ok;icmp type echo-request
meta l4proto 1 icmp type echo-request;ok;icmp type echo-request
icmp type echo-request;ok
+meta protocol ip udp dport 67;ok
+meta protocol ip6 udp dport 67;ok
> > meta ibrname "br0";fail
> > meta obrname "br0";fail
> >
> > diff --git a/tests/py/ip/meta.t.json b/tests/py/ip/meta.t.json
> > index f83864f672d5..3df31ce381fc 100644
> > --- a/tests/py/ip/meta.t.json
> > +++ b/tests/py/ip/meta.t.json
> > @@ -140,3 +140,19 @@
> > "accept": null
> > }
> > ]
> > +
> > +# meta protocol ip udp dport 67
> > +[
> > + {
> > + "match": {
> > + "left": {
> > + "payload": {
> > + "field": "dport",
> > + "protocol": "udp"
> > + }
> > + },
> > + "op": "==",
> > + "right": 67
> > + }
> > + }
> > +]
>
> This translation is not correct, the 'meta protocol' match is missing in
> JSON. Even though it is not present when listing ruleset, the idea is
> that JSON is identical to standard syntax input to avoid any "short
> cuts" (I guess it's just a typo, other spots are fine).
If I understand correctly the json tests, this actually checking for
the listing output, which has already removed the 'meta protocol'
match.
IIRC, if I add 'meta protocol' here, the json tests report a warning.
> [...]
> > diff --git a/tests/py/ip6/meta.t b/tests/py/ip6/meta.t
> > index dce97f5b0fd0..2c1aee2309a9 100644
> > --- a/tests/py/ip6/meta.t
> > +++ b/tests/py/ip6/meta.t
> > @@ -9,5 +9,8 @@ meta l4proto icmp icmp type echo-request;ok;icmp type echo-request
> > meta l4proto 1 icmp type echo-request;ok;icmp type echo-request
> > icmp type echo-request;ok
> >
> > +meta protocol ip udp dport 67;ok
> > +meta protocol ip6 udp dport 67;ok
> > +
>
> Here, 'meta protocol ip6' should be dropped.
No, it is checking that this is removed. Note that this spews a
warning in tests/py when running tests.
Look: If we want to fix this right(tm), we should update
payload_try_merge() to actually remove this redundant dependency at
rule load time, instead of doing this lazy dependency removal at
listing time.
> Pablo, do you want to have another look at your patch or should I try to
> fix things up?
Sure, you can follow up to refine. You're welcome.
Thanks for reviewing.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill
2021-08-30 17:21 ` Pablo Neira Ayuso
@ 2021-08-30 17:53 ` Pablo Neira Ayuso
0 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-08-30 17:53 UTC (permalink / raw)
To: Phil Sutter, netfilter-devel, Florian Westphal
On Mon, Aug 30, 2021 at 07:21:14PM +0200, Pablo Neira Ayuso wrote:
[...]
> Look: If we want to fix this right(tm), we should update
> payload_try_merge() to actually remove this redundant dependency at
> rule load time, instead of doing this lazy dependency removal at
> listing time.
Or actually, this match statement could be just canceled from the
evaluation phase for these cases:
ip family meta protocol ip
ip6 family meta protocol ip6
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-30 17:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-26 10:49 [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill Pablo Neira Ayuso
2021-08-26 10:59 ` Florian Westphal
2021-08-30 16:40 ` Phil Sutter
2021-08-30 17:21 ` Pablo Neira Ayuso
2021-08-30 17:53 ` 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.