[ovs-dev] [PATCH ovn] lex: Allow unmasked bits in value/mask tokens.

[Dumitru Ceara]

On 6/23/20 4:56 PM, Mark Michelson wrote:
> Acked-by: Mark Michelson <mmichels at redhat.com>
> 

Thanks Mark for the review.

> I was going to suggest printing an informational message if the value
> gets truncated. However, given the way the lexer works, it's not as
> simple as just printing a message when you encounter the situation. If
> we were to add informational messages to the lexer, that should be a
> separate (and very low-priority) patch.

I was looking into that too but couldn't find a not-so-intrusive and
clean way to do it. Do you have any suggestions? I can try to send a
follow up patch if we think it's worth it.

Thanks,
Dumitru

> 
> On 6/23/20 4:17 AM, Dumitru Ceara wrote:
>> It's quite restrictive to not accept ACLs/policies that match on a CIDR
>> that has non-zero host bits. Right now this generates a lexer error that
>> can only be detected in the logs.
>>
>> There's no real harm in automatically zero-ing the unmasked bits.
>>
>> Reported-at: https://bugzilla.redhat.com/1812820
>> Reported-by: Ying Xu <yinxu at redhat.com>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>>   lib/lex.c    | 10 ++--------
>>   tests/ovn.at |  8 ++++----
>>   2 files changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/lib/lex.c b/lib/lex.c
>> index 94f6c77..4d92199 100644
>> --- a/lib/lex.c
>> +++ b/lib/lex.c
>> @@ -485,16 +485,10 @@ lex_parse_mask(const char *p, struct lex_token
>> *token)
>>           return p;
>>       }
>>   -    /* Check invariant that a 1-bit in the value corresponds to a
>> 1-bit in the
>> +    /* Apply invariant that a 1-bit in the value corresponds to a
>> 1-bit in the
>>        * mask. */
>>       for (int i = 0; i < ARRAY_SIZE(token->mask.be32); i++) {
>> -        ovs_be32 v = token->value.be32[i];
>> -        ovs_be32 m = token->mask.be32[i];
>> -
>> -        if (v & ~m) {
>> -            lex_error(token, "Value contains unmasked 1-bits.");
>> -            break;
>> -        }
>> +        token->value.be32[i] &= token->mask.be32[i];
>>       }
>>         /* Done! */
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 1ff7952..0c0daed 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -79,7 +79,7 @@ a/b => a error("`/' is only valid as part of `//' or
>> `/*'.") b
>>     0/0
>>   0/1
>> -1/0 => error("Value contains unmasked 1-bits.")
>> +1/0 => 0/0
>>   1/1
>>   128/384
>>   1/3
>> @@ -99,7 +99,7 @@ a/b => a error("`/' is only valid as part of `//' or
>> `/*'.") b
>>   0X => error("Hex digits expected following 0X.")
>>   0x0/0x0 => 0/0
>>   0x0/0x1 => 0/0x1
>> -0x1/0x0 => error("Value contains unmasked 1-bits.")
>> +0x1/0x0 => 0/0
>>   0xffff/0x1ffff
>>   0x. => error("Invalid syntax in hexadecimal constant.")
>>   @@ -109,7 +109,7 @@ a/b => a error("`/' is only valid as part of
>> `//' or `/*'.") b
>>   192.168.0.0/255.255.0.0 => 192.168.0.0/16
>>   192.168.0.0/255.255.255.0 => 192.168.0.0/24
>>   192.168.0.0/255.255.0.255
>> -192.168.0.0/255.0.0.0 => error("Value contains unmasked 1-bits.")
>> +192.168.0.0/255.0.0.0 => 192.0.0.0/8
>>   192.168.0.0/32
>>   192.168.0.0/255.255.255.255 => 192.168.0.0/32
>>   1.2.3.4:5 => 1.2.3.4 : 5
>> @@ -135,7 +135,7 @@ FE:DC:ba:98:76:54 => fe:dc:ba:98:76:54
>>   01:00:00:00:00:00/01:00:00:00:00:00
>>   ff:ff:ff:ff:ff:ff/ff:ff:ff:ff:ff:ff
>>   fe:ff:ff:ff:ff:ff/ff:ff:ff:ff:ff:ff
>> -ff:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff => error("Value contains unmasked
>> 1-bits.")
>> +ff:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff =>
>> fe:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff
>>   fe:x => error("Invalid numeric constant.")
>>   00:01:02:03:04:x => error("Invalid numeric constant.")
>>  
>