[nft,1/8] tests: adjust output to silence warnings

Message ID 20171026230611.14269-2-fw@strlen.de
State Under Review
Delegated to: Pablo Neira
Headers show
Series
  • rework dependency removal
Related show

Commit Message

Florian Westphal Oct. 26, 2017, 11:06 p.m.
silence following (correct but harmless) warnings:
bridge/icmpX.t: WARNING: line: 6: 'src/nft add rule --debug=netlink bridge test-bridge input icmp type echo-request': 'icmp type echo-request' mismatches 'ether type ip icmp type echo-request'
bridge/icmpX.t: WARNING: line: 8: 'src/nft add rule --debug=netlink bridge test-bridge input icmpv6 type echo-request': 'icmpv6 type echo-request' mismatches 'ether type ip6 icmpv6 type echo-request'
inet/icmpX.t: WARNING: line: 6: 'src/nft add rule --debug=netlink inet test-inet input icmp type echo-request': 'icmp type echo-request' mismatches 'meta nfproto ipv4 icmp type echo-request'
inet/icmpX.t: WARNING: line: 8: 'src/nft add rule --debug=netlink inet test-inet input icmpv6 type echo-request': 'icmpv6 type echo-request' mismatches 'meta nfproto ipv6 icmpv6 type echo-request'

in all of these cases, we *could* remove the dependency, it can
be correctly re-built using icmp/icmpv6.

However, at this time, nft dependency removal does not have the needed
information to do this correctly.  In order to remove the ll dependency
(ether type, meta nfproto) we would need to know if the layer 4 protocol
is icmp (implies ipv4) or icmpv6 (implies ipv6).

Access to the next expression (meta l4proto) is NOT enough, for example:
ether type ip meta l4proto tcp

does not imply ip, we would need access to the rhs (the layer 4
protocol number) to know if the layer 2 check is (was) implied by another
statement later on.

To do that we would need two passes over a rule, or we would need to
track dependencies per-base.

So for now just accept that we don't handle it.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/bridge/icmpX.t | 4 ++--
 tests/py/inet/icmpX.t   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Pablo Neira Ayuso Oct. 27, 2017, 10:29 a.m. | #1
Hi Florian,

On Fri, Oct 27, 2017 at 01:06:04AM +0200, Florian Westphal wrote:
> silence following (correct but harmless) warnings:
> bridge/icmpX.t: WARNING: line: 6: 'src/nft add rule --debug=netlink bridge test-bridge input icmp type echo-request': 'icmp type echo-request' mismatches 'ether type ip icmp type echo-request'
> bridge/icmpX.t: WARNING: line: 8: 'src/nft add rule --debug=netlink bridge test-bridge input icmpv6 type echo-request': 'icmpv6 type echo-request' mismatches 'ether type ip6 icmpv6 type echo-request'
> inet/icmpX.t: WARNING: line: 6: 'src/nft add rule --debug=netlink inet test-inet input icmp type echo-request': 'icmp type echo-request' mismatches 'meta nfproto ipv4 icmp type echo-request'
> inet/icmpX.t: WARNING: line: 8: 'src/nft add rule --debug=netlink inet test-inet input icmpv6 type echo-request': 'icmpv6 type echo-request' mismatches 'meta nfproto ipv6 icmpv6 type echo-request'

Hm, I'm not hitting this here, probably there's a bug in test
infrastructure.

> in all of these cases, we *could* remove the dependency, it can
> be correctly re-built using icmp/icmpv6.
> 
> However, at this time, nft dependency removal does not have the needed
> information to do this correctly.  In order to remove the ll dependency
> (ether type, meta nfproto) we would need to know if the layer 4 protocol
> is icmp (implies ipv4) or icmpv6 (implies ipv6).
> 
> Access to the next expression (meta l4proto) is NOT enough, for example:
> ether type ip meta l4proto tcp
> 
> does not imply ip, we would need access to the rhs (the layer 4
> protocol number) to know if the layer 2 check is (was) implied by another
> statement later on.
> 
> To do that we would need two passes over a rule, or we would need to
> track dependencies per-base.
> 
> So for now just accept that we don't handle it.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  tests/py/bridge/icmpX.t | 4 ++--
>  tests/py/inet/icmpX.t   | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/py/bridge/icmpX.t b/tests/py/bridge/icmpX.t
> index 4d7b9b0637aa..58e366c00712 100644
> --- a/tests/py/bridge/icmpX.t
> +++ b/tests/py/bridge/icmpX.t
> @@ -3,6 +3,6 @@
>  *bridge;test-bridge;input
>  
>  ip protocol icmp icmp type echo-request;ok;icmp type echo-request
> -icmp type echo-request;ok
> +icmp type echo-request;ok;ether type ip icmp type echo-request
>  ip6 nexthdr icmpv6 icmpv6 type echo-request;ok;ip6 nexthdr 58 icmpv6 type echo-request
> -icmpv6 type echo-request;ok
> +icmpv6 type echo-request;ok;ether type ip6 icmpv6 type echo-request
> diff --git a/tests/py/inet/icmpX.t b/tests/py/inet/icmpX.t
> index 43ac0909195f..91f7b9e1c472 100644
> --- a/tests/py/inet/icmpX.t
> +++ b/tests/py/inet/icmpX.t
> @@ -3,8 +3,8 @@
>  *inet;test-inet;input
>  
>  ip protocol icmp icmp type echo-request;ok;icmp type echo-request
> -icmp type echo-request;ok
> +icmp type echo-request;ok;meta nfproto ipv4 icmp type echo-request

I read a couple of times your description above and I must be
overlooking anything.

To me, "icmp type echo-request" in bridge/inet/netdev should result in
two implicit dependencies, so this ends up looking like this:

1) check for IPv4, then...
2) check for ICMP in iph->protocol, then...
3) check for ICMP type.

This would be the default reasonable behaviour.

Then, we have to deal with specific corner cases, where we should
cancel dependencies.

Am I missing anything?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal Oct. 27, 2017, 12:41 p.m. | #2
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Florian,
> 
> On Fri, Oct 27, 2017 at 01:06:04AM +0200, Florian Westphal wrote:
> > silence following (correct but harmless) warnings:
> > bridge/icmpX.t: WARNING: line: 6: 'src/nft add rule --debug=netlink bridge test-bridge input icmp type echo-request': 'icmp type echo-request' mismatches 'ether type ip icmp type echo-request'
> > bridge/icmpX.t: WARNING: line: 8: 'src/nft add rule --debug=netlink bridge test-bridge input icmpv6 type echo-request': 'icmpv6 type echo-request' mismatches 'ether type ip6 icmpv6 type echo-request'
> > inet/icmpX.t: WARNING: line: 6: 'src/nft add rule --debug=netlink inet test-inet input icmp type echo-request': 'icmp type echo-request' mismatches 'meta nfproto ipv4 icmp type echo-request'
> > inet/icmpX.t: WARNING: line: 8: 'src/nft add rule --debug=netlink inet test-inet input icmpv6 type echo-request': 'icmpv6 type echo-request' mismatches 'meta nfproto ipv6 icmpv6 type echo-request'
> 
> Hm, I'm not hitting this here, probably there's a bug in test
> infrastructure.

The test case is new, most likely git pull will help.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal Oct. 27, 2017, 12:52 p.m. | #3
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > index 43ac0909195f..91f7b9e1c472 100644
> > --- a/tests/py/inet/icmpX.t
> > +++ b/tests/py/inet/icmpX.t
> > @@ -3,8 +3,8 @@
> >  *inet;test-inet;input
> >  
> >  ip protocol icmp icmp type echo-request;ok;icmp type echo-request
> > -icmp type echo-request;ok
> > +icmp type echo-request;ok;meta nfproto ipv4 icmp type echo-request
> 
> I read a couple of times your description above and I must be
> overlooking anything.
> 
> To me, "icmp type echo-request" in bridge/inet/netdev should result in
> two implicit dependencies, so this ends up looking like this:
> 
> 1) check for IPv4, then...
> 2) check for ICMP in iph->protocol, then...
> 3) check for ICMP type.
> 
> This would be the default reasonable behaviour.
> 
> Then, we have to deal with specific corner cases, where we should
> cancel dependencies.
> 
> Am I missing anything?

Sorry, I overlooked this on my first reply.

Your assesment is correct, that is indeed the default reasonable
behaviour, but, when removing, we have limited information on
the rule at the moment.

So this is really:

1) check for IPv4, then...
2) check for some l4 protocol

... from a dependency removal perspective.
and 2) doesn't provide enough information to decide if the dependency
is needed or not.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Oct. 27, 2017, 2:07 p.m. | #4
On Fri, Oct 27, 2017 at 02:52:02PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > index 43ac0909195f..91f7b9e1c472 100644
> > > --- a/tests/py/inet/icmpX.t
> > > +++ b/tests/py/inet/icmpX.t
> > > @@ -3,8 +3,8 @@
> > >  *inet;test-inet;input
> > >  
> > >  ip protocol icmp icmp type echo-request;ok;icmp type echo-request
> > > -icmp type echo-request;ok
> > > +icmp type echo-request;ok;meta nfproto ipv4 icmp type echo-request
> > 
> > I read a couple of times your description above and I must be
> > overlooking anything.
> > 
> > To me, "icmp type echo-request" in bridge/inet/netdev should result in
> > two implicit dependencies, so this ends up looking like this:
> > 
> > 1) check for IPv4, then...
> > 2) check for ICMP in iph->protocol, then...
> > 3) check for ICMP type.
> > 
> > This would be the default reasonable behaviour.
> > 
> > Then, we have to deal with specific corner cases, where we should
> > cancel dependencies.
> > 
> > Am I missing anything?
> 
> Sorry, I overlooked this on my first reply.
> 
> Your assesment is correct, that is indeed the default reasonable
> behaviour, but, when removing, we have limited information on
> the rule at the moment.
>
> So this is really:
> 
> 1) check for IPv4, then...
> 2) check for some l4 protocol
> 
> ... from a dependency removal perspective.
> and 2) doesn't provide enough information to decide if the dependency
> is needed or not.

We probably need to make an initial pass of the entire rule, populate
context, then kill these dependencies once we have a global view on
what is being expressed there. Make sense to you?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal Oct. 27, 2017, 6:03 p.m. | #5
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Oct 27, 2017 at 02:52:02PM +0200, Florian Westphal wrote:
> We probably need to make an initial pass of the entire rule, populate
> context, then kill these dependencies once we have a global view on
> what is being expressed there. Make sense to you?

Yes, unfortunately I won't have time to work on that anytime soon.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/tests/py/bridge/icmpX.t b/tests/py/bridge/icmpX.t
index 4d7b9b0637aa..58e366c00712 100644
--- a/tests/py/bridge/icmpX.t
+++ b/tests/py/bridge/icmpX.t
@@ -3,6 +3,6 @@ 
 *bridge;test-bridge;input
 
 ip protocol icmp icmp type echo-request;ok;icmp type echo-request
-icmp type echo-request;ok
+icmp type echo-request;ok;ether type ip icmp type echo-request
 ip6 nexthdr icmpv6 icmpv6 type echo-request;ok;ip6 nexthdr 58 icmpv6 type echo-request
-icmpv6 type echo-request;ok
+icmpv6 type echo-request;ok;ether type ip6 icmpv6 type echo-request
diff --git a/tests/py/inet/icmpX.t b/tests/py/inet/icmpX.t
index 43ac0909195f..91f7b9e1c472 100644
--- a/tests/py/inet/icmpX.t
+++ b/tests/py/inet/icmpX.t
@@ -3,8 +3,8 @@ 
 *inet;test-inet;input
 
 ip protocol icmp icmp type echo-request;ok;icmp type echo-request
-icmp type echo-request;ok
+icmp type echo-request;ok;meta nfproto ipv4 icmp type echo-request
 ip6 nexthdr icmpv6 icmpv6 type echo-request;ok;ip6 nexthdr 58 icmpv6 type echo-request
-icmpv6 type echo-request;ok
+icmpv6 type echo-request;ok;meta nfproto ipv6 icmpv6 type echo-request
 # must not remove 'ip protocol' dependency, this explicitly matches icmpv6-in-ipv4.
 ip protocol ipv6-icmp meta l4proto ipv6-icmp icmpv6 type 1;ok;ip protocol 58 meta l4proto 58 icmpv6 type destination-unreachable