Re: [PATCH nft] netlink_delinearize: incorrect meta protocol dependency kill

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Phil,

On Mon, Aug 30, 2021 at 06:40:41PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Thu, Aug 26, 2021 at 12:49:52PM +0200, Pablo Neira Ayuso wrote:
> > meta protocol is meaningful in bridge, netdev and inet familiiess, do
> > not remove this.
> 
> So you want to avoid dependency killing in above families, but:
> 
> [...]
> > diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> > index 5b545701e8b7..92617a46df6f 100644
> > --- a/src/netlink_delinearize.c
> > +++ b/src/netlink_delinearize.c
> [...]
> > @@ -2005,7 +2005,22 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
> >  	case NFPROTO_BRIDGE:
> >  		break;
> >  	default:
> > -		return true;
> > +		if (dep->left->etype != EXPR_META ||
> > +		    dep->right->etype != EXPR_VALUE)
> > +			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;
> >  	}
> >  
> >  	if (expr->left->meta.key != NFT_META_L4PROTO)
> 
> The above code only applies to families other than inet, netdev or
> bridge?! Am I missing something?

Yes, this code only applies to ip and ip6, this removes "meta
protocol" in this cases:

 rule ip x y meta protocol ip udp dport 67 => udp dport 67
 rule ip6 x y meta protocol ip6 udp dport 67 => udp dport 67

I should have keep it in a separated patch actually since it is not
related to bridge/inet/netdev. Sorry for the noise.

> AFAIU, dependency killing defaults to true and examines the cases where
> it should not happen. So the old behaviour was for unexpected dependency
> expressions to just drop them instead of keeping them. The code you add
> above does the opposite. That's not wrong per se, but I assume there was
> a reason why dependency elimination was implemented "greedy", therefore
> I kept it that way when refactoring the code.
> 
> > @@ -2015,7 +2030,8 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
> >  
> >  	switch (dep->left->etype) {
> >  	case EXPR_META:
> > -		if (dep->left->meta.key != NFT_META_NFPROTO)
> > +		if (dep->left->meta.key != NFT_META_NFPROTO &&
> > +		    dep->left->meta.key != NFT_META_PROTOCOL)
> >  			return true;
> >  		break;
> >  	case EXPR_PAYLOAD:
> 
> If we continue evaluation for 'meta protocol' payload dependencies, RHS
> values need adjustment: with 'meta nfproto' we'll see NFPROTO_IPV4 or
> NFPROTO_IPV6 while with 'meta protocol' we'll see ETH_P_IP or
> ETH_P_IPV6. The EXPR_PAYLOAD case has such conversion code.

The chunk above the actual fix for the reported issue.

Yes, checking for ETH_P_IP and ETH_P_IPV6 would be good to narrow down
the dependency killing more precisely.

> [...]
> > diff --git a/tests/py/ip/meta.t b/tests/py/ip/meta.t
> > index f733d22de2c3..fecd0caf71a7 100644
> > --- a/tests/py/ip/meta.t
> > +++ b/tests/py/ip/meta.t
> > @@ -8,6 +8,8 @@ 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
> > +
> 
> Hmm. Shouldn't this drop the dependency, i.e. list as 'udp dport 67' as
> the family is defined by table family already?

It does actually, this spews a warning if you run tests/py.

> There should also be a case for 'meta protocol ip6 udp dport 67' which
> makes sure the dependency is not dropped, right?

Yes, I forgot to add it to ip/meta.t

I added it to ip6/meta.t

--- a/tests/py/ip6/meta.t
+++ b/tests/py/ip6/meta.t
@@ -9,5 +9,8 @@ meta l4proto icmp icmp type echo-request;ok;icmp type echo-request
 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 ibrname "br0";fail
> >  meta obrname "br0";fail
> >  
> > diff --git a/tests/py/ip/meta.t.json b/tests/py/ip/meta.t.json
> > index f83864f672d5..3df31ce381fc 100644
> > --- a/tests/py/ip/meta.t.json
> > +++ b/tests/py/ip/meta.t.json
> > @@ -140,3 +140,19 @@
> >          "accept": null
> >      }
> >  ]
> > +
> > +# meta protocol ip udp dport 67
> > +[
> > +    {
> > +        "match": {
> > +            "left": {
> > +                "payload": {
> > +                    "field": "dport",
> > +                    "protocol": "udp"
> > +                }
> > +            },
> > +            "op": "==",
> > +            "right": 67
> > +        }
> > +    }
> > +]
> 
> This translation is not correct, the 'meta protocol' match is missing in
> JSON. Even though it is not present when listing ruleset, the idea is
> that JSON is identical to standard syntax input to avoid any "short
> cuts" (I guess it's just a typo, other spots are fine).

If I understand correctly the json tests, this actually checking for
the listing output, which has already removed the 'meta protocol'
match.

IIRC, if I add 'meta protocol' here, the json tests report a warning.

> [...]
> > diff --git a/tests/py/ip6/meta.t b/tests/py/ip6/meta.t
> > index dce97f5b0fd0..2c1aee2309a9 100644
> > --- a/tests/py/ip6/meta.t
> > +++ b/tests/py/ip6/meta.t
> > @@ -9,5 +9,8 @@ meta l4proto icmp icmp type echo-request;ok;icmp type echo-request
> >  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
> > +
> 
> Here, 'meta protocol ip6' should be dropped.

No, it is checking that this is removed. Note that this spews a
warning in tests/py when running tests.

Look: If we want to fix this right(tm), we should update
payload_try_merge() to actually remove this redundant dependency at
rule load time, instead of doing this lazy dependency removal at
listing time.

> Pablo, do you want to have another look at your patch or should I try to
> fix things up?

Sure, you can follow up to refine. You're welcome.

Thanks for reviewing.