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

[Mark Michelson]

On 6/23/20 11:08 AM, Dumitru Ceara wrote:
> 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.

I gave it about 30 seconds of thought and couldn't think of anything 
non-intrusive either. I also have to admit I don't have a ton of 
experience in the lexer code, so it would take some research to figure 
out a good way to change this.

I don't think it's worth following up immediately with a separate patch. 
I think this gets filed into our "it would be nice to have" pile.

> 
> 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.")
>>>   
>>
>