[ovs-dev] [PATCH] nx-match: Only store significant bytes to stack.

Jarno Rajahalme jarno at ovn.org
Fri Jan 6 21:10:14 UTC 2017


> On Jan 5, 2017, at 9:06 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Thu, Jan 05, 2017 at 05:45:29PM -0800, Jarno Rajahalme wrote:
>> 
>>> On Jan 5, 2017, at 4:48 PM, Ben Pfaff <blp at ovn.org> wrote:
>>> 
>>> On Thu, Jan 05, 2017 at 04:03:17PM -0800, Jarno Rajahalme wrote:
>>>> 
>>>>> On Jan 4, 2017, at 11:03 PM, Ben Pfaff <blp at ovn.org> wrote:
>>>>> 
>>>>> On Wed, Jan 04, 2017 at 07:21:44PM -0800, Jarno Rajahalme wrote:
>>>>>> 
>>>>>>> On Dec 21, 2016, at 2:36 PM, Ben Pfaff <blp at ovn.org> wrote:
>>>>>>> 
>>>>>>> On Wed, Dec 07, 2016 at 04:49:00PM -0800, Jarno Rajahalme wrote:
>>>>>>> I'd be more comfortable if nx_stack_pop() had assertions to check for
>>>>>>> underflow.
>>>>>> 
>>>>>> I don’t think OVS should assert fail if controller issues one pop
>>>>>> too many? Do you mean that current users of nx_stack_pop() do not
>>>>>> check for NULL return? I had a look and think that setting “*bytes”
>>>>>> to zero when returning NULL should be enough for all users.
>>>>> 
>>>>> It appears to me that if stack->size is greater than 0 but less than the
>>>>> number of bytes indicated by its last byte, then it will corrupt the
>>>>> ofpbuf size and that this will later cause some kind of failure that
>>>>> will be harder to debug than an assertion failure. 
>>>>> 
>>>> 
>>>> OK, now i got it.  This is just to guard against (future) bugs in OVS itself.
>>> 
>>> Yes.
>>> 
>>>>>>> In ofputil_decode_packet_in_private(), it's probably worth checking the
>>>>>>> format of the stack we pull from the payload, since a badly formatted
>>>>>>> stack can segfault us (if we leave out assertions) or assert-fail us (if
>>>>>>> we include them).
>>>>>>> 
>>>>>> 
>>>>>> What do you mean with badly formatted stack? Zero-sized property? IMO
>>>>>> even that would be properly pushed and popped from the stack, storing
>>>>>> only the length (of zero) in the stack.
>>>>> 
>>>>> I mean that if the property contains, for example, a single byte with
>>>>> value 0xff, then it's badly formatted because we can pop off a length
>>>>> (255) but then popping off that number of bytes will underflow.
>>>> 
>>>> I did not change the encoding of the stack as properties, so each
>>>> value in the stack is still encoded as a separate property, where the
>>>> (aligned) value length is used as the property length. 
>>> 
>>> I guess I forgot that.
>>> 
>>> Thanks, that's fine then.
>>> 
>>>> ofpprop_pull() does the length checking for the properties and the
>>>> current code in ofputil_decode_packet_in_private() assert fails on any
>>>> error, which is not good, as a controller bug would crash OVS?
>>> 
>>> That's bad.  Maybe the fix is as simple as this, though.
>>> 
>>> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
>>> index 156d8d2..421b9d7 100644
>>> --- a/lib/ofp-util.c
>>> +++ b/lib/ofp-util.c
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
>>> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc.
>>> *
>>> * Licensed under the Apache License, Version 2.0 (the "License");
>>> * you may not use this file except in compliance with the License.
>>> @@ -4061,7 +4061,9 @@ ofputil_decode_packet_in_private(const struct ofp_header *oh, bool loose,
>>>        uint64_t type;
>>> 
>>>        error = ofpprop_pull(&continuation, &payload, &type);
>>> -        ovs_assert(!error);
>>> +        if (error) {
>>> +            break;
>>> +        }
>>> 
>>>        switch (type) {
>>>        case NXCPT_BRIDGE:
>>> @@ -4124,7 +4126,7 @@ ofputil_decode_packet_in_private(const struct ofp_header *oh, bool loose,
>>>        ofputil_packet_in_private_destroy(pin);
>>>    }
>>> 
>>> -    return 0;
>>> +    return error;
>>> }
>>> 
>>> /* Frees data in 'pin' that is dynamically allocated by
>>> 
>> 
>> I folded this in to v3.
> 
> This bug is in 2.6, isn't it?  If so then we should fix it in a separate
> patch, for backporting purposes.

Right, I’ll separate it out. What is the most proper way to attribute it to you? "Suggested-by”, or something else?

  Jarno



More information about the dev mailing list