[ovs-dev] [PATCH v3 RESEND] classifier: Prevent tries vs n_tries race leading to NULL dereference.

Eiichi Tsukata eiichi.tsukata at nutanix.com
Wed May 27 01:37:31 UTC 2020


Hello Ilya

Thanks for comments!
Replied inline.

> On May 27, 2020, at 1:28, Ilya Maximets <i.maximets at ovn.org> wrote:
> 
> On 5/18/20 1:06 AM, Eiichi Tsukata wrote:
>> 
>> 
>> diff --git a/lib/classifier.c b/lib/classifier.c
>> index 0fad953..10909a6 100644
>> --- a/lib/classifier.c
>> +++ b/lib/classifier.c
>> @@ -393,7 +393,9 @@ classifier_set_prefix_fields(struct classifier *cls,
>>         bitmap_set1(fields.bm, trie_fields[i]);
>> 
>>         new_fields[n_tries] = NULL;
>> -        if (n_tries >= cls->n_tries || field != cls->tries[n_tries].field) {
>> +        const struct mf_field *cls_field
>> +            = ovsrcu_get(struct mf_field *, &cls->tries[n_tries].field);
>> +        if (n_tries >= cls->n_tries || field != cls_field) {
>>             new_fields[n_tries] = field;
>>             changed = true;
>>         }
>> @@ -454,7 +456,7 @@ trie_init(struct classifier *cls, int trie_idx, const struct mf_field *field)
>>     } else {
>>         ovsrcu_set_hidden(&trie->root, NULL);
>>     }
>> -    trie->field = field;
>> +    ovsrcu_set_hidden(&trie->field, field);
> 
> CONST_CAST required for the 'field' argument.  Otherwise build fails
> due to discarded const qualifier.

Sorry, I’ll fix it in v4. Many thanks.

>> 
>>     /* Ports trie. */
>> @@ -1577,8 +1579,10 @@ check_tries(struct trie_ctx trie_ctx[CLS_MAX_TRIES], unsigned int n_tries,
>>     for (j = 0; j < n_tries; j++) {
>>         /* Is the trie field relevant for this subtable, and
>>            is the trie field within the current range of fields? */
>> -        if (field_plen[j] &&
>> -            flowmap_is_set(&range_map, trie_ctx[j].be32ofs / 2)) {
>> +        const struct mf_field *ctx_field
>> +            = ovsrcu_get(struct mf_field *, &trie_ctx[j].trie->field);
>> +        if (field_plen[j] && ctx_field &&
> 
> 
> Looking at this part one more time I see that you didn't actually change
> the order of reads (like it was in v1).  trie->filed is still read before
> checking the field_plen.
> 
> RCU protects from the NULL pointer dereference here, so it should be fine,
> but the code remains logically incorrect in terms that writer still thinks
> that order of reads is opposite.
> 
> Not sure if we need to fix that in this patch, though.  What do you think?
> 
> 

I think I should fix it in this patch. The behavior should be fine but it can confuse us.
I’ll change the read order in v4.

Best Regards

Eiichi



More information about the dev mailing list