[PATCH nft 0/4] restrict meta nfproto to inet family

[PATCH nft 0/4] restrict meta nfproto to inet family

[Florian Westphal]

Pablo reports following test case failure:

any/ct.t: ERROR: line 94: src/nft add rule --debug=netlink ip6
test-ip6 output meta nfproto ipv4 ct original saddr 1.2.3.4: This rule should not have failed.

We can't find upper layer protocol in this case, but even if we'd
"fix" this it is still non-sensical, as

  meta nfproto ipv4

will never match except in the inet family and the
ip family, but in the latter case it will always match so it
has no effect).

So, first step is to move this to an inet specific test to
get rid of the test case failure.

The followup changes then get rid of meta nfproto tests or
move them to inet-family-only tests.

The last patch makes nft reject 'meta nfproto' in all families
except inet, where this expression is needed in case one wants to
explicitly restrict a rule to only ipv4 or ipv6.

[PATCH 1/4] tests: restrict ct saddr test to inet family

[Florian Westphal]

any/ct.t: ERROR: line 94: src/nft add rule --debug=netlink ip6
test-ip6 output meta nfproto ipv4 ct original saddr 1.2.3.4: This rule should not have failed.

Actually, this failure is "ok; we can't find upper layer protocol
in this case, but even if we'd "fix" this it is still non-sensical,
meta nfproto ipv4, but family is ipv6 --> rule would never match.

First move this to an inet-specific test.
A followup patch will reject meta nfproto for all families except inet.

Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/any/ct.t          |  1 -
 tests/py/any/ct.t.payload  |  7 -------
 tests/py/inet/ct.t         | 10 ++++++++++
 tests/py/inet/ct.t.payload | 13 +++++++++++++
 4 files changed, 23 insertions(+), 8 deletions(-)
 create mode 100644 tests/py/inet/ct.t
 create mode 100644 tests/py/inet/ct.t.payload

diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
index 20f047a2963b..3a58162d2846 100644
--- a/tests/py/any/ct.t
+++ b/tests/py/any/ct.t
@@ -91,7 +91,6 @@ ct bytes original reply;fail
 # missing direction
 ct saddr 1.2.3.4;fail
 
-meta nfproto ipv4 ct original saddr 1.2.3.4;ok
 # wrong base (ip6 but ipv4 address given)
 meta nfproto ipv6 ct original saddr 1.2.3.4;fail
 
diff --git a/tests/py/any/ct.t.payload b/tests/py/any/ct.t.payload
index 80513c803557..20acbb9b63d1 100644
--- a/tests/py/any/ct.t.payload
+++ b/tests/py/any/ct.t.payload
@@ -373,13 +373,6 @@ ip test-ip4 output
   [ byteorder reg 1 = hton(reg 1, 8, 8) ]
   [ cmp lt reg 1 0x00000000 0xf4010000 ]
 
-# meta nfproto ipv4 ct original saddr 1.2.3.4
-ip test-ip4 output
-  [ meta load nfproto => reg 1 ]
-  [ cmp eq reg 1 0x00000002 ]
-  [ ct load src => reg 1 , dir original ]
-  [ cmp eq reg 1 0x04030201 ]
-
 # ct status expected,seen-reply,assured,confirmed,snat,dnat,dying
 ip test-ip4 output
   [ ct load status => reg 1 ]
diff --git a/tests/py/inet/ct.t b/tests/py/inet/ct.t
new file mode 100644
index 000000000000..c56c3bc86151
--- /dev/null
+++ b/tests/py/inet/ct.t
@@ -0,0 +1,10 @@
+:input;type filter hook input priority 0
+:ingress;type filter hook ingress device lo priority 0
+
+*inet;test-inet;input
+
+meta nfproto ipv4 ct original saddr 1.2.3.4;ok
+meta nfproto ipv6 ct original saddr ::1;ok
+
+# missing protocol context
+ct original saddr ::1;fail
diff --git a/tests/py/inet/ct.t.payload b/tests/py/inet/ct.t.payload
new file mode 100644
index 000000000000..21c74581de3a
--- /dev/null
+++ b/tests/py/inet/ct.t.payload
@@ -0,0 +1,13 @@
+# meta nfproto ipv4 ct original saddr 1.2.3.4
+ip test-ip4 output
+  [ meta load nfproto => reg 1 ]
+  [ cmp eq reg 1 0x00000002 ]
+  [ ct load src => reg 1 , dir original ]
+  [ cmp eq reg 1 0x04030201 ]
+
+# meta nfproto ipv6 ct original saddr ::1
+inet test-inet input
+  [ meta load nfproto => reg 1 ]
+  [ cmp eq reg 1 0x0000000a ]
+  [ ct load src => reg 1 , dir original ]
+  [ cmp eq reg 1 0x00000000 0x00000000 0x00000000 0x01000000 ]
-- 
2.13.0

[PATCH 2/4] tests: remove two non-sensical rules

[Florian Westphal]

meta nfproto returns the hook family that the current packet
is being evaluted in, e.g.
NFPROTO_NETDEV in case we're called from the netdev context.

This makes no sense, if we add a rule to netdev, bridge, ip, ...
table then thats where it will be evaluated, no runtime test needed.

Only exception: inet family, in this case, nfproto will be either
ipv4 or ipv6.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/inet/ether.t         |  2 --
 tests/py/inet/ether.t.payload | 23 -----------------------
 2 files changed, 25 deletions(-)

diff --git a/tests/py/inet/ether.t b/tests/py/inet/ether.t
index 0a88aef1609b..afdf8b89ca65 100644
--- a/tests/py/inet/ether.t
+++ b/tests/py/inet/ether.t
@@ -7,9 +7,7 @@
 *bridge;test-bridge;input
 *netdev;test-netdev;ingress
 
-tcp dport 22 iiftype ether ether saddr 00:0f:54:0c:11:4 meta nfproto ipv4 accept;ok;tcp dport 22 ether saddr 00:0f:54:0c:11:04 meta nfproto ipv4 accept
 tcp dport 22 iiftype ether ether saddr 00:0f:54:0c:11:4 accept;ok;tcp dport 22 ether saddr 00:0f:54:0c:11:04 accept
 tcp dport 22 ether saddr 00:0f:54:0c:11:04 accept;ok
 
 ether saddr 00:0f:54:0c:11:04 accept;ok
-ether saddr 00:0f:54:0c:11:04 meta nfproto ipv4;ok
diff --git a/tests/py/inet/ether.t.payload b/tests/py/inet/ether.t.payload
index 86f30c37f6bd..53648413d588 100644
--- a/tests/py/inet/ether.t.payload
+++ b/tests/py/inet/ether.t.payload
@@ -1,17 +1,3 @@
-# tcp dport 22 iiftype ether ether saddr 00:0f:54:0c:11:4 meta nfproto ipv4 accept
-inet test-inet input
-  [ meta load l4proto => reg 1 ]
-  [ cmp eq reg 1 0x00000006 ]
-  [ payload load 2b @ transport header + 2 => reg 1 ]
-  [ cmp eq reg 1 0x00001600 ]
-  [ meta load iiftype => reg 1 ]
-  [ cmp eq reg 1 0x00000001 ]
-  [ payload load 6b @ link header + 6 => reg 1 ]
-  [ cmp eq reg 1 0x0c540f00 0x00000411 ]
-  [ meta load nfproto => reg 1 ]
-  [ cmp eq reg 1 0x00000002 ]
-  [ immediate reg 0 accept ]
-
 # tcp dport 22 iiftype ether ether saddr 00:0f:54:0c:11:4 accept
 inet test-inet input
   [ meta load l4proto => reg 1 ]
@@ -44,12 +30,3 @@ inet test-inet input
   [ cmp eq reg 1 0x0c540f00 0x00000411 ]
   [ immediate reg 0 accept ]
 
-# ether saddr 00:0f:54:0c:11:04 meta nfproto ipv4
-inet test-inet input
-  [ meta load iiftype => reg 1 ]
-  [ cmp eq reg 1 0x00000001 ]
-  [ payload load 6b @ link header + 6 => reg 1 ]
-  [ cmp eq reg 1 0x0c540f00 0x00000411 ]
-  [ meta load nfproto => reg 1 ]
-  [ cmp eq reg 1 0x00000002 ]
-
-- 
2.13.0

[PATCH 3/4] tests: restrict meta nfproto test cases to inet family

[Florian Westphal]

Followup patch will reject meta nfproto for non-inet families.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/any/meta.t                      |  5 -----
 tests/py/any/meta.t.payload              | 26 --------------------------
 tests/py/inet/meta.t                     | 10 ++++++++++
 tests/py/inet/meta.t.payload             | 25 +++++++++++++++++++++++++
 tests/shell/testcases/listing/0011sets_0 |  2 +-
 5 files changed, 36 insertions(+), 32 deletions(-)
 create mode 100644 tests/py/inet/meta.t
 create mode 100644 tests/py/inet/meta.t.payload

diff --git a/tests/py/any/meta.t b/tests/py/any/meta.t
index 2ff942ff039d..80a0f29bce48 100644
--- a/tests/py/any/meta.t
+++ b/tests/py/any/meta.t
@@ -25,11 +25,6 @@ meta protocol != {ip, arp, ip6, vlan};ok
 meta protocol ip;ok
 meta protocol != ip;ok
 
-meta nfproto ipv4;ok
-meta nfproto ipv6;ok
-meta nfproto {ipv4, ipv6};ok
-meta nfproto != {ipv4, ipv6};ok
-
 meta l4proto 22;ok
 meta l4proto != 233;ok
 meta l4proto 33-45;ok
diff --git a/tests/py/any/meta.t.payload b/tests/py/any/meta.t.payload
index 871f1ada5abe..b2065f3d920b 100644
--- a/tests/py/any/meta.t.payload
+++ b/tests/py/any/meta.t.payload
@@ -104,32 +104,6 @@ ip test-ip4 input
   [ meta load protocol => reg 1 ]
   [ cmp neq reg 1 0x00000008 ]
 
-# meta nfproto ipv4
-ip test-ip4 input
-  [ meta load nfproto => reg 1 ]
-  [ cmp eq reg 1 0x00000002 ]
-
-# meta nfproto ipv6
-ip test-ip4 input
-  [ meta load nfproto => reg 1 ]
-  [ cmp eq reg 1 0x0000000a ]
-
-# meta nfproto {ipv4, ipv6}
-__set%d test-ip4 3
-__set%d test-ip4 0
-	element 00000002  : 0 [end]	element 0000000a  : 0 [end]
-ip test-ip4 input
-  [ meta load nfproto => reg 1 ]
-  [ lookup reg 1 set __set%d ]
-
-# meta nfproto != {ipv4, ipv6}
-__set%d test-ip4 3
-__set%d test-ip4 0
-	element 00000002  : 0 [end]	element 0000000a  : 0 [end]
-ip test-ip4 input
-  [ meta load nfproto => reg 1 ]
-  [ lookup reg 1 set __set%d 0x1 ]
-
 # meta l4proto 22
 ip test-ip4 input
   [ meta load l4proto => reg 1 ]
diff --git a/tests/py/inet/meta.t b/tests/py/inet/meta.t
new file mode 100644
index 000000000000..723dd46cc6ba
--- /dev/null
+++ b/tests/py/inet/meta.t
@@ -0,0 +1,10 @@
+:input;type filter hook input priority 0
+:ingress;type filter hook ingress device lo priority 0
+
+*inet;test-inet;input
+
+meta nfproto ipv4;ok
+meta nfproto ipv6;ok
+meta nfproto {ipv4, ipv6};ok
+meta nfproto != {ipv4, ipv6};ok
+
diff --git a/tests/py/inet/meta.t.payload b/tests/py/inet/meta.t.payload
new file mode 100644
index 000000000000..8b6759a11c36
--- /dev/null
+++ b/tests/py/inet/meta.t.payload
@@ -0,0 +1,25 @@
+# meta nfproto ipv4
+ip test-ip4 input
+  [ meta load nfproto => reg 1 ]
+  [ cmp eq reg 1 0x00000002 ]
+
+# meta nfproto ipv6
+ip test-ip4 input
+  [ meta load nfproto => reg 1 ]
+  [ cmp eq reg 1 0x0000000a ]
+
+# meta nfproto {ipv4, ipv6}
+__set%d test-ip4 3
+__set%d test-ip4 0
+	element 00000002  : 0 [end]	element 0000000a  : 0 [end]
+ip test-ip4 input
+  [ meta load nfproto => reg 1 ]
+  [ lookup reg 1 set __set%d ]
+
+# meta nfproto != {ipv4, ipv6}
+__set%d test-ip4 3
+__set%d test-ip4 0
+	element 00000002  : 0 [end]	element 0000000a  : 0 [end]
+ip test-ip4 input
+  [ meta load nfproto => reg 1 ]
+  [ lookup reg 1 set __set%d 0x1 ]
diff --git a/tests/shell/testcases/listing/0011sets_0 b/tests/shell/testcases/listing/0011sets_0
index 75f2895ff7e5..f021962a3881 100755
--- a/tests/shell/testcases/listing/0011sets_0
+++ b/tests/shell/testcases/listing/0011sets_0
@@ -25,7 +25,7 @@ $NFT add rule ip6 test test udp sport {123}
 
 $NFT add table arp test_arp
 $NFT add chain arp test_arp test
-$NFT add rule arp test_arp test meta nfproto {ipv4}
+$NFT add rule arp test_arp test meta mark {123}
 
 $NFT add table bridge test_bridge
 $NFT add chain bridge test_bridge test
-- 
2.13.0

[PATCH 4/4] evaluate: reject meta nfproto outside of inet family

[Florian Westphal]

meta nfproto loads the hook family type of the current rule context
in the kernel, i.e. it will be NFPROTO_IPV6 for ip6 family,
NFPROTO_BRIDGE for bridge and so on.

The only case where this is useful is the inet pseudo family,
where this is useful to determine the real hook family
(NFPROTO_IPV4 or NFPROTO_IPV6).

In all other families 'meta nfproto' is either always true or false.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 doc/nft.xml    |  9 ++++++++-
 src/evaluate.c | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/doc/nft.xml b/doc/nft.xml
index d0d37396dddf..e9ccd63c7164 100644
--- a/doc/nft.xml
+++ b/doc/nft.xml
@@ -478,7 +478,9 @@ filter input iif $int_ifs accept
 			</simplelist>.
 
 			The <literal>inet</literal> address family is a dummy family which is used to create
-			hybrid IPv4/IPv6 tables.
+			hybrid IPv4/IPv6 tables.  The <literal>meta</literal> expression <literal>nfproto</literal>
+			keyword can be used to test which family (ipv4 or ipv6) context the packet is being processed in.
+
 
 			When no address family is specified, <literal>ip</literal> is used by default.
 		</para>
@@ -1907,6 +1909,11 @@ filter output icmpv6 type { echo-request, echo-reply }
 								<entry>integer (32 bit)</entry>
 							</row>
 							<row>
+								<entry>nfproto</entry>
+								<entry>real hook protocol family, useful only in inet table</entry>
+								<entry>integer (32 bit)</entry>
+							</row>
+							<row>
 								<entry>protocol</entry>
 								<entry>Ethertype protocol value</entry>
 								<entry>ether_type</entry>
diff --git a/src/evaluate.c b/src/evaluate.c
index 311c86c5abe9..4cbbd5f9b52a 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1691,6 +1691,18 @@ static int expr_evaluate_fib(struct eval_ctx *ctx, struct expr **exprp)
 	return expr_evaluate_primary(ctx, exprp);
 }
 
+static int expr_evaluate_meta(struct eval_ctx *ctx, struct expr **exprp)
+{
+	struct expr *meta = *exprp;
+
+	if (ctx->pctx.family != NFPROTO_INET &&
+	    meta->flags & EXPR_F_PROTOCOL &&
+	    meta->meta.key == NFT_META_NFPROTO)
+		return expr_error(ctx->msgs, meta,
+					  "meta nfproto is only useful in the inet family");
+	return expr_evaluate_primary(ctx, exprp);
+}
+
 static int expr_evaluate(struct eval_ctx *ctx, struct expr **expr)
 {
 #ifdef DEBUG
@@ -1712,8 +1724,9 @@ static int expr_evaluate(struct eval_ctx *ctx, struct expr **expr)
 	case EXPR_EXTHDR:
 		return expr_evaluate_exthdr(ctx, expr);
 	case EXPR_VERDICT:
-	case EXPR_META:
 		return expr_evaluate_primary(ctx, expr);
+	case EXPR_META:
+		return expr_evaluate_meta(ctx, expr);
 	case EXPR_FIB:
 		return expr_evaluate_fib(ctx, expr);
 	case EXPR_PAYLOAD:
-- 
2.13.0

Re: [PATCH 4/4] evaluate: reject meta nfproto outside of inet family

[Pablo Neira Ayuso]

On Fri, Jun 16, 2017 at 10:34:11PM +0200, Florian Westphal wrote:
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 311c86c5abe9..4cbbd5f9b52a 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1691,6 +1691,18 @@ static int expr_evaluate_fib(struct eval_ctx *ctx, struct expr **exprp)
>  	return expr_evaluate_primary(ctx, exprp);
>  }
>  
> +static int expr_evaluate_meta(struct eval_ctx *ctx, struct expr **exprp)
> +{
> +	struct expr *meta = *exprp;
> +
> +	if (ctx->pctx.family != NFPROTO_INET &&
> +	    meta->flags & EXPR_F_PROTOCOL &&
> +	    meta->meta.key == NFT_META_NFPROTO)
> +		return expr_error(ctx->msgs, meta,
> +					  "meta nfproto is only useful in the inet family");

Nitpick: Before you push this one, you probably want to mangle this
line to get it aligned with expr_error().

Thanks!

Re: [PATCH nft 0/4] restrict meta nfproto to inet family

[Pablo Neira Ayuso]

On Fri, Jun 16, 2017 at 10:34:07PM +0200, Florian Westphal wrote:
> Pablo reports following test case failure:
> 
> any/ct.t: ERROR: line 94: src/nft add rule --debug=netlink ip6
> test-ip6 output meta nfproto ipv4 ct original saddr 1.2.3.4: This rule should not have failed.
> 
> We can't find upper layer protocol in this case, but even if we'd
> "fix" this it is still non-sensical, as
> 
>   meta nfproto ipv4
> 
> will never match except in the inet family and the
> ip family, but in the latter case it will always match so it
> has no effect).
> 
> So, first step is to move this to an inet specific test to
> get rid of the test case failure.
> 
> The followup changes then get rid of meta nfproto tests or
> move them to inet-family-only tests.
> 
> The last patch makes nft reject 'meta nfproto' in all families
> except inet, where this expression is needed in case one wants to
> explicitly restrict a rule to only ipv4 or ipv6.

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks Florian.

Can we probably get rid of meta nfproto at some point? IIRC it's only
needed because skb->protocol is not set in the output hook, but it is
indeed available in postrouting. Can you find any reason for this
behaviour?