The original intent was to fix a bug: The following rule in inet table:
| meta nfproto ipv4 icmpv6 type echo-reply
Was added correctly but when printing the meta match was falsely
removed. The fix is to deny dependency killing if RHS family of nfproto
match doesn't match RHS family of l4proto match. Adding this to the
already large conditional led to even more unreadable code, therefore
this patch tries to clean that up (and also removes the partial code
duplication.
Signed-off-by: Phil Sutter <phil@xxxxxx>
---
When trying to test/fix this for bridge family, I noticed a few oddities
I am unsure how to clean up:
First of all, plain statements such as:
| icmp type echo-request
| icmpv6 type echo-request
create the l3proto dependency via payload expression. Should they use
'meta protocol' expression instead? Does it matter at all or is one of
the alternatives superior performance-wise? Note that this payload
expression is not merged with the one for transport header, there is a
'meta l4proto' one in between. For example:
| # icmp type echo-request
| bridge test-bridge output
| [ payload load 2b @ link header + 12 => reg 1 ]
| [ cmp eq reg 1 0x00000008 ]
| [ meta load l4proto => reg 1 ]
| [ cmp eq reg 1 0x00000001 ]
| [ payload load 1b @ transport header + 0 => reg 1 ]
| [ cmp eq reg 1 0x00000008 ]
Now when using 'meta protocol' expression to match "unusual"
combinations (e.g., icmp over IPv6), the code generates never matching
rules:
| # meta protocol ip6 icmp type echo-request
| bridge test-bridge output
| [ meta load protocol => reg 1 ]
| [ cmp eq reg 1 0x0000dd86 ]
| [ payload load 2b @ link header + 12 => reg 1 ]
| [ cmp eq reg 1 0x00000008 ]
| [ meta load l4proto => reg 1 ]
| [ cmp eq reg 1 0x00000001 ]
| [ payload load 1b @ transport header + 0 => reg 1 ]
| [ cmp eq reg 1 0x00000008 ]
Unless I'm mistaken, this rule will only match if EtherType is 0x86dd
(via first 'meta protocol' expression) and 0x0800 (via implicitly added
payload expression).
If I use an ether expression, things seem to work fine though:
| # nft --debug=netlink add rule bridge t c ether type ip icmpv6 type echo-request
| bridge t c
| [ payload load 2b @ link header + 12 => reg 1 ]
| [ cmp eq reg 1 0x00000008 ]
| [ meta load l4proto => reg 1 ]
| [ cmp eq reg 1 0x0000003a ]
| [ payload load 1b @ transport header + 0 => reg 1 ]
| [ cmp eq reg 1 0x00000080 ]
So how can this be solved? Extend dependency adding to catch 'meta
protocol' expression as well as 'ether type' one? Assuming that one of
the two is to be preferred, convert one of them into the other?
Cheers, Phil
---
src/netlink_delinearize.c | 83 +++++++++++-----------
tests/py/inet/icmp.t | 18 +++++
tests/py/inet/icmp.t.json | 114 +++++++++++++++++++++++++++++++
tests/py/inet/icmp.t.json.output | 30 ++++++++
tests/py/inet/icmp.t.payload | 54 +++++++++++++++
5 files changed, 259 insertions(+), 40 deletions(-)
create mode 100644 tests/py/inet/icmp.t
create mode 100644 tests/py/inet/icmp.t.json
create mode 100644 tests/py/inet/icmp.t.json.output
create mode 100644 tests/py/inet/icmp.t.payload
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 31d62420d41c8..7e9765cfa016a 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1522,61 +1522,64 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
const struct expr *expr)
{
struct expr *dep = ctx->pdep->expr;
+ uint16_t l3proto;
+ uint8_t l4proto;
if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
return true;
switch (family) {
case NFPROTO_INET:
- switch (dep->left->ops->type) {
- case EXPR_META:
- if (dep->left->meta.key == NFT_META_NFPROTO &&
- (mpz_get_uint16(dep->right->value) == NFPROTO_IPV4 ||
- mpz_get_uint16(dep->right->value) == NFPROTO_IPV6) &&
- expr->left->meta.key == NFT_META_L4PROTO &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
- return false;
- break;
- case EXPR_PAYLOAD:
- if (dep->left->payload.base == PROTO_BASE_LL_HDR &&
- (mpz_get_uint16(dep->right->value) == ETH_P_IP ||
- mpz_get_uint16(dep->right->value) == ETH_P_IPV6) &&
- expr->left->meta.key == NFT_META_L4PROTO &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
- return false;
- break;
- default:
- break;
- }
- break;
case NFPROTO_NETDEV:
case NFPROTO_BRIDGE:
- switch (dep->left->ops->type) {
- case EXPR_META:
- if (dep->left->meta.key == NFT_META_PROTOCOL &&
- (mpz_get_uint16(dep->right->value) == ETH_P_IP ||
- mpz_get_uint16(dep->right->value) == ETH_P_IPV6) &&
- expr->left->meta.key == NFT_META_L4PROTO &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
- return false;
+ break;
+ default:
+ return true;
+ }
+
+ if (expr->left->meta.key != NFT_META_L4PROTO)
+ return true;
+
+ l3proto = mpz_get_uint16(dep->right->value);
+
+ switch (dep->left->ops->type) {
+ case EXPR_META:
+ if (dep->left->meta.key != NFT_META_NFPROTO)
+ 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 EXPR_PAYLOAD:
- if (dep->left->payload.base == PROTO_BASE_LL_HDR &&
- (mpz_get_uint16(dep->right->value) == ETH_P_IP ||
- mpz_get_uint16(dep->right->value) == ETH_P_IPV6) &&
- expr->left->meta.key == NFT_META_L4PROTO &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMP &&
- mpz_get_uint8(expr->right->value) != IPPROTO_ICMPV6)
- return false;
+ case ETH_P_IPV6:
+ l3proto = NFPROTO_IPV6;
break;
default:
break;
}
break;
+ default:
+ break;
+ }
+
+ l4proto = mpz_get_uint8(expr->right->value);
+
+ switch (l4proto) {
+ case IPPROTO_ICMP:
+ case IPPROTO_ICMPV6:
+ break;
+ default:
+ return false;
}
+
+ if ((l3proto == NFPROTO_IPV4 && l4proto == IPPROTO_ICMPV6) ||
+ (l3proto == NFPROTO_IPV6 && l4proto == IPPROTO_ICMP))
+ return false;
+
return true;
}
diff --git a/tests/py/inet/icmp.t b/tests/py/inet/icmp.t
new file mode 100644
index 0000000000000..9014f846729c7
--- /dev/null
+++ b/tests/py/inet/icmp.t
@@ -0,0 +1,18 @@
+:output;type filter hook output priority 0
+
+*inet;test-inet;output
+
+# without nfproto specified, these should add an implicit dependency on
+# the likely l3 proto (i.e., IPv6 for icmpv6 and IPv4 for icmp)
+
+icmp type echo-request;ok
+icmpv6 type echo-request;ok
+
+# make sure only those nfproto matches are dropped if
+# the next statement would add it as a dependency anyway
+
+meta nfproto ipv4 icmp type echo-request;ok;icmp type echo-request
+meta nfproto ipv4 icmpv6 type echo-request;ok
+
+meta nfproto ipv6 icmp type echo-request;ok
+meta nfproto ipv6 icmpv6 type echo-request;ok;icmpv6 type echo-request
diff --git a/tests/py/inet/icmp.t.json b/tests/py/inet/icmp.t.json
new file mode 100644
index 0000000000000..c4517605a7186
--- /dev/null
+++ b/tests/py/inet/icmp.t.json
@@ -0,0 +1,114 @@
+# icmp type echo-request
+[
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmp"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
+# icmpv6 type echo-request
+[
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmpv6"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
+# meta nfproto ipv4 icmp type echo-request
+[
+ {
+ "match": {
+ "left": { "meta": "nfproto" },
+ "right": "ipv4"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmp"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
+# meta nfproto ipv4 icmpv6 type echo-request
+[
+ {
+ "match": {
+ "left": { "meta": "nfproto" },
+ "right": "ipv4"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmpv6"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
+# meta nfproto ipv6 icmp type echo-request
+[
+ {
+ "match": {
+ "left": { "meta": "nfproto" },
+ "right": "ipv6"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmp"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
+# meta nfproto ipv6 icmpv6 type echo-request
+[
+ {
+ "match": {
+ "left": { "meta": "nfproto" },
+ "right": "ipv6"
+ }
+ },
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmpv6"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
diff --git a/tests/py/inet/icmp.t.json.output b/tests/py/inet/icmp.t.json.output
new file mode 100644
index 0000000000000..2282900d58e98
--- /dev/null
+++ b/tests/py/inet/icmp.t.json.output
@@ -0,0 +1,30 @@
+# meta nfproto ipv4 icmp type echo-request
+[
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmp"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
+# meta nfproto ipv6 icmpv6 type echo-request
+[
+ {
+ "match": {
+ "left": {
+ "payload": {
+ "field": "type",
+ "name": "icmpv6"
+ }
+ },
+ "right": "echo-request"
+ }
+ }
+]
+
diff --git a/tests/py/inet/icmp.t.payload b/tests/py/inet/icmp.t.payload
new file mode 100644
index 0000000000000..f98cfc39abed4
--- /dev/null
+++ b/tests/py/inet/icmp.t.payload
@@ -0,0 +1,54 @@
+# icmp type echo-request
+inet test-inet output
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x00000002 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000001 ]
+ [ payload load 1b @ transport header + 0 => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+
+# icmpv6 type echo-request
+inet test-inet output
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x0000000a ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x0000003a ]
+ [ payload load 1b @ transport header + 0 => reg 1 ]
+ [ cmp eq reg 1 0x00000080 ]
+
+# meta nfproto ipv4 icmp type echo-request
+inet test-inet output
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x00000002 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000001 ]
+ [ payload load 1b @ transport header + 0 => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+
+# meta nfproto ipv4 icmpv6 type echo-request
+inet test-inet output
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x00000002 ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x0000003a ]
+ [ payload load 1b @ transport header + 0 => reg 1 ]
+ [ cmp eq reg 1 0x00000080 ]
+
+# meta nfproto ipv6 icmp type echo-request
+inet test-inet output
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x0000000a ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x00000001 ]
+ [ payload load 1b @ transport header + 0 => reg 1 ]
+ [ cmp eq reg 1 0x00000008 ]
+
+# meta nfproto ipv6 icmpv6 type echo-request
+inet test-inet output
+ [ meta load nfproto => reg 1 ]
+ [ cmp eq reg 1 0x0000000a ]
+ [ meta load l4proto => reg 1 ]
+ [ cmp eq reg 1 0x0000003a ]
+ [ payload load 1b @ transport header + 0 => reg 1 ]
+ [ cmp eq reg 1 0x00000080 ]
+
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
