> On Apr 16, 2016, at 10:08 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> On Sat, Apr 16, 2016 at 09:28:40PM -0700, Jarno Rajahalme wrote:
>> Addition of table versioning exposed struct cls_rule member
>> 'cls_match' to RCU readers and made it possible for
>> 'cls_match' become NULL while being accessed by an RCU reader, but we failed
>> to check for this condition. This may have resulted in NULL pointer
>> dereference and ovs-vswitchd crash.
>>
>> Fix this by making the 'cls_match' member an RCU pointer and checking
>> the value whenever it potentially read by an RCU reader. In these
>> instances we use ovsrcu_get(), whereas functions accessible only by
>> the exclusive writers use ovsrcu_get_protected() and do not need to
>> check the result.
>>
>> VMware-BZ: 1643642
>> Fixes: 2b7b1427 ("classifier: Support table versioning")
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
>
> Thank you for finding this!
>
> I read the code carefully. I also built it (on top of master) with
> Clang 3.5 and GCC 4.9.1 (for i386), getting clean compiles and no
> warnings from "sparse" either. I also ran the testsuite and it passed.
>
Thanks, I did not have the time last night to do all that!
> I think that most of the changes to classifier.h are just reordering the
> function prototypes. I don't know why this order is being changed.
> Maybe it is to group the comments better? I don't know whether the
> reordering is properly part of a minimal fix (since this will require
> backporting); I'll leave that to your judgment.
>
I wanted it to be clearer which functions are to be called by the exclusive
writer only, and which are for the RCU readers. I separated this re-organizing
to a separate patch (which need not be backported).
> This new comment in classifier.h could use a space at the end:
>
> /* Classifier rule properties. These are RCU protected and may run
> * concurrently with modifiers and each other.*/
>
Thanks!
> I think that the change to classifier_rule_overlaps() causes it to
> inline cls_rule_visible_in_version(), so that it could be made clearer
> by just calling that function directly.
>
Right, thanks for spotting this.
> The number of instances, with or without _protected, of
> ovsrcu_get(struct cls_match *, &rule->cls_match)
> verges on wanting a pair of helper functions, especially since it would
> probably reduce the amount of line-breaking. Again I leave it to your
> judgment.
>
I added helpers to classifier-private.h so that both classifier.c and
test-classifier.c can share them.
> Acked-by: Ben Pfaff <b...@ovn.org <mailto:b...@ovn.org>>
>
Pushed to master, and backported to branches 2.5 and 2.4.
Jarno
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
