| Message ID | 20171026230611.14269-2-fw@strlen.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | rework dependency removal | expand |
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
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
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
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
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
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
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(-)