* [PATCH nft 1/2] netlink_delinearize: incorrect meta protocol dependency kill again
@ 2021-08-30 22:34 Pablo Neira Ayuso
2021-08-30 22:34 ` [PATCH nft 2/2] rule: remove redundant meta protocol from the evaluation step Pablo Neira Ayuso
0 siblings, 1 reply; 2+ messages in thread
From: Pablo Neira Ayuso @ 2021-08-30 22:34 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
This patch adds __meta_dependency_may_kill() to consolidate inspection
of the meta protocol, nfproto and ether type expression to validate
dependency removal on listings.
Phil reports that 567ea4774e13 includes an update on the ip and ip6
families that is not described in the patch, moreover, it flips the
default verdict from true to false.
Fixes: 567ea4774e13 ("netlink_delinearize: incorrect meta protocol dependency kill")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/netlink_delinearize.c | 106 ++++++++++--------
.../testcases/optimizations/dependency_kill | 48 ++++++++
.../optimizations/dumps/dependency_kill.nft | 42 +++++++
3 files changed, 151 insertions(+), 45 deletions(-)
create mode 100755 tests/shell/testcases/optimizations/dependency_kill
create mode 100644 tests/shell/testcases/optimizations/dumps/dependency_kill.nft
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 92617a46df6f..0f00b9555266 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1983,6 +1983,55 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
}
}
+static uint8_t ether_type_to_nfproto(uint16_t l3proto)
+{
+ switch(l3proto) {
+ case ETH_P_IP:
+ return NFPROTO_IPV4;
+ case ETH_P_IPV6:
+ return NFPROTO_IPV6;
+ default:
+ break;
+ }
+
+ return NFPROTO_UNSPEC;
+}
+
+static bool __meta_dependency_may_kill(const struct expr *dep, uint8_t *nfproto)
+{
+ uint16_t l3proto;
+
+ switch (dep->left->etype) {
+ case EXPR_META:
+ switch (dep->left->meta.key) {
+ case NFT_META_NFPROTO:
+ *nfproto = mpz_get_uint8(dep->right->value);
+ break;
+ case NFT_META_PROTOCOL:
+ l3proto = mpz_get_uint16(dep->right->value);
+ *nfproto = ether_type_to_nfproto(l3proto);
+ break;
+ default:
+ return true;
+ }
+ break;
+ case EXPR_PAYLOAD:
+ if (dep->left->payload.base != PROTO_BASE_LL_HDR)
+ return true;
+
+ if (dep->left->dtype != ðertype_type)
+ return true;
+
+ l3proto = mpz_get_uint16(dep->right->value);
+ *nfproto = ether_type_to_nfproto(l3proto);
+ break;
+ default:
+ break;
+ }
+
+ return false;
+}
+
/* We have seen a protocol key expression that restricts matching at the network
* base, leave it in place since this is meaninful in bridge, inet and netdev
* families. Exceptions are ICMP and ICMPv6 where this code assumes that can
@@ -1993,66 +2042,33 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
const struct expr *expr)
{
struct expr *dep = ctx->pdep->expr;
- uint16_t l3proto, protocol;
- uint8_t l4proto;
+ uint8_t l4proto, nfproto;
if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
return true;
+ if (__meta_dependency_may_kill(dep, &nfproto))
+ return true;
+
switch (family) {
case NFPROTO_INET:
case NFPROTO_NETDEV:
case NFPROTO_BRIDGE:
break;
default:
- if (dep->left->etype != EXPR_META ||
- dep->right->etype != EXPR_VALUE)
+ if (family == NFPROTO_IPV4 &&
+ nfproto != NFPROTO_IPV4)
+ return false;
+ else if (family == NFPROTO_IPV6 &&
+ nfproto != NFPROTO_IPV6)
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;
+ return true;
}
if (expr->left->meta.key != NFT_META_L4PROTO)
return true;
- l3proto = mpz_get_uint16(dep->right->value);
-
- switch (dep->left->etype) {
- case EXPR_META:
- if (dep->left->meta.key != NFT_META_NFPROTO &&
- dep->left->meta.key != NFT_META_PROTOCOL)
- return true;
- break;
- case EXPR_PAYLOAD:
- if (dep->left->payload.base != PROTO_BASE_LL_HDR)
- return true;
-
- switch(l3proto) {
- case ETH_P_IP:
- l3proto = NFPROTO_IPV4;
- break;
- case ETH_P_IPV6:
- l3proto = NFPROTO_IPV6;
- break;
- default:
- break;
- }
- break;
- default:
- break;
- }
-
l4proto = mpz_get_uint8(expr->right->value);
switch (l4proto) {
@@ -2063,8 +2079,8 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
return false;
}
- if ((l3proto == NFPROTO_IPV4 && l4proto == IPPROTO_ICMPV6) ||
- (l3proto == NFPROTO_IPV6 && l4proto == IPPROTO_ICMP))
+ if ((nfproto == NFPROTO_IPV4 && l4proto == IPPROTO_ICMPV6) ||
+ (nfproto == NFPROTO_IPV6 && l4proto == IPPROTO_ICMP))
return false;
return true;
diff --git a/tests/shell/testcases/optimizations/dependency_kill b/tests/shell/testcases/optimizations/dependency_kill
new file mode 100755
index 000000000000..904eecf89b6e
--- /dev/null
+++ b/tests/shell/testcases/optimizations/dependency_kill
@@ -0,0 +1,48 @@
+#!/bin/bash
+
+set -e
+
+RULESET="table bridge foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table ip foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table ip6 foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table netdev foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table inet foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ meta nfproto ipv4 udp dport 67
+ meta nfproto ipv6 udp dport 67
+ }
+}"
+
+$NFT -f - <<< $RULESET
diff --git a/tests/shell/testcases/optimizations/dumps/dependency_kill.nft b/tests/shell/testcases/optimizations/dumps/dependency_kill.nft
new file mode 100644
index 000000000000..1781f7be0c09
--- /dev/null
+++ b/tests/shell/testcases/optimizations/dumps/dependency_kill.nft
@@ -0,0 +1,42 @@
+table bridge foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table ip foo {
+ chain bar {
+ udp dport 67
+ meta protocol ip6 udp dport 67
+ udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table ip6 foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ udp dport 67
+ ether type ip udp dport 67
+ udp dport 67
+ }
+}
+table netdev foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ }
+}
+table inet foo {
+ chain bar {
+ meta protocol ip udp dport 67
+ meta protocol ip6 udp dport 67
+ ether type ip udp dport 67
+ ether type ip6 udp dport 67
+ meta nfproto ipv4 udp dport 67
+ meta nfproto ipv6 udp dport 67
+ }
+}
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [PATCH nft 2/2] rule: remove redundant meta protocol from the evaluation step
2021-08-30 22:34 [PATCH nft 1/2] netlink_delinearize: incorrect meta protocol dependency kill again Pablo Neira Ayuso
@ 2021-08-30 22:34 ` Pablo Neira Ayuso
0 siblings, 0 replies; 2+ messages in thread
From: Pablo Neira Ayuso @ 2021-08-30 22:34 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
567ea4774e13 ("netlink_delinearize: incorrect meta protocol dependency kill")
does not document two cases that are handled in this patch:
- 'meta protocol ip' is removed if used in the ip family.
- 'meta protocol ip6' is removed if used in the ip6 family.
This patch removes this redundancy earlier, from the evaluation step
before netlink bytecode generation.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/rule.c | 77 ++++++++++++++++++++++++++-----------
tests/py/ip/meta.t | 2 +-
tests/py/ip/meta.t.payload | 2 -
tests/py/ip6/meta.t | 2 +-
tests/py/ip6/meta.t.payload | 2 -
5 files changed, 56 insertions(+), 29 deletions(-)
diff --git a/src/rule.c b/src/rule.c
index 3e59f27c69be..6091067f608b 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2757,49 +2757,80 @@ static void payload_do_merge(struct stmt *sa[], unsigned int n)
}
/**
- * payload_try_merge - try to merge consecutive payload match statements
+ * stmt_reduce - reduce statements in rule
*
* @rule: nftables rule
*
+ * This function aims to:
+ *
+ * - remove redundant statement, e.g. remove 'meta protocol ip' if family is ip
+ * - merge consecutive payload match statements
+ *
* Locate sequences of payload match statements referring to adjacent
* header locations and merge those using only equality relations.
*
* As a side-effect, payload match statements are ordered in ascending
* order according to the location of the payload.
*/
-static void payload_try_merge(const struct rule *rule)
+static void stmt_reduce(const struct rule *rule)
{
+ struct stmt *stmt, *dstmt = NULL, *next;
struct stmt *sa[rule->num_stmts];
- struct stmt *stmt, *next;
unsigned int idx = 0;
list_for_each_entry_safe(stmt, next, &rule->stmts, list) {
+ /* delete this redundant statement */
+ if (dstmt) {
+ list_del(&dstmt->list);
+ stmt_free(dstmt);
+ dstmt = NULL;
+ }
+
/* Must not merge across other statements */
- if (stmt->ops->type != STMT_EXPRESSION)
- goto do_merge;
+ if (stmt->ops->type != STMT_EXPRESSION) {
+ if (idx < 2)
+ continue;
- if (stmt->expr->etype != EXPR_RELATIONAL)
+ payload_do_merge(sa, idx);
+ idx = 0;
continue;
- if (stmt->expr->left->etype != EXPR_PAYLOAD)
+ }
+
+ if (stmt->expr->etype != EXPR_RELATIONAL)
continue;
if (stmt->expr->right->etype != EXPR_VALUE)
continue;
- switch (stmt->expr->op) {
- case OP_EQ:
- case OP_IMPLICIT:
- case OP_NEQ:
- break;
- default:
- continue;
- }
- sa[idx++] = stmt;
- continue;
-do_merge:
- if (idx < 2)
- continue;
- payload_do_merge(sa, idx);
- idx = 0;
+ if (stmt->expr->left->etype == EXPR_PAYLOAD) {
+ switch (stmt->expr->op) {
+ case OP_EQ:
+ case OP_IMPLICIT:
+ case OP_NEQ:
+ break;
+ default:
+ continue;
+ }
+
+ sa[idx++] = stmt;
+ } else if (stmt->expr->left->etype == EXPR_META) {
+ switch (stmt->expr->op) {
+ case OP_EQ:
+ case OP_IMPLICIT:
+ if (stmt->expr->left->meta.key == NFT_META_PROTOCOL) {
+ uint16_t protocol;
+
+ protocol = mpz_get_uint16(stmt->expr->right->value);
+ if ((rule->handle.family == NFPROTO_IPV4 &&
+ protocol == ETH_P_IP) ||
+ (rule->handle.family == NFPROTO_IPV6 &&
+ protocol == ETH_P_IPV6))
+ dstmt = stmt;
+ }
+ break;
+ default:
+ break;
+ }
+ }
}
if (idx > 1)
@@ -2808,6 +2839,6 @@ do_merge:
struct error_record *rule_postprocess(struct rule *rule)
{
- payload_try_merge(rule);
+ stmt_reduce(rule);
return NULL;
}
diff --git a/tests/py/ip/meta.t b/tests/py/ip/meta.t
index fecd0caf71a7..5a05923a1ce1 100644
--- a/tests/py/ip/meta.t
+++ b/tests/py/ip/meta.t
@@ -8,7 +8,7 @@ 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 protocol ip udp dport 67;ok;udp dport 67
meta ibrname "br0";fail
meta obrname "br0";fail
diff --git a/tests/py/ip/meta.t.payload b/tests/py/ip/meta.t.payload
index a1fd00864ef9..afde5cc13ac5 100644
--- a/tests/py/ip/meta.t.payload
+++ b/tests/py/ip/meta.t.payload
@@ -47,8 +47,6 @@ ip6 test-ip4 input
# meta protocol ip udp dport 67
ip test-ip4 input
- [ 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 ]
diff --git a/tests/py/ip6/meta.t b/tests/py/ip6/meta.t
index 2c1aee2309a9..471e14811975 100644
--- a/tests/py/ip6/meta.t
+++ b/tests/py/ip6/meta.t
@@ -10,7 +10,7 @@ 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 protocol ip6 udp dport 67;ok;udp dport 67
meta sdif "lo" accept;ok
meta sdifname != "vrf1" accept;ok
diff --git a/tests/py/ip6/meta.t.payload b/tests/py/ip6/meta.t.payload
index 59c20d994138..0e3db6ba07f9 100644
--- a/tests/py/ip6/meta.t.payload
+++ b/tests/py/ip6/meta.t.payload
@@ -56,8 +56,6 @@ ip6 test-ip6 input
# meta protocol ip6 udp dport 67
ip6 test-ip6 input
- [ 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 ]
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-30 22:34 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30 22:34 [PATCH nft 1/2] netlink_delinearize: incorrect meta protocol dependency kill again Pablo Neira Ayuso
2021-08-30 22:34 ` [PATCH nft 2/2] rule: remove redundant meta protocol from the evaluation step 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).