[ovs-dev] [PATCH] classifier: Fix race condition leading to NULL dereference.

[Jarno Rajahalme]

> On Apr 16, 2016, at 10:08 PM, Ben Pfaff <blp at 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 <jarno at 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 <blp at ovn.org <mailto:blp at ovn.org>>
> 

Pushed to master, and backported to branches 2.5 and 2.4.

  Jarno

> Thanks,
> 
> Ben.