> On Apr 18, 2020, at 1:14, Ilya Maximets <[email protected]> wrote: > > On 4/15/20 2:24 AM, Eiichi Tsukata wrote: >> Thanks for comments, Ilya. >> >>> On Apr 14, 2020, at 21:11, Ilya Maximets <[email protected]> wrote: >>> >>> On 4/14/20 6:00 AM, Eiichi Tsukata wrote: >>>> >>>> >>>> @@ -1575,11 +1573,16 @@ check_tries(struct trie_ctx >>>> trie_ctx[CLS_MAX_TRIES], unsigned int n_tries, >>>> * fields using the prefix tries. The trie checks are done only as >>>> * needed to avoid folding in additional bits to the wildcards mask. */ >>>> 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)) { >>>> - struct trie_ctx *ctx = &trie_ctx[j]; >>>> + struct trie_ctx *ctx = &trie_ctx[j]; >>>> + /* Check if trie field is relevant for this subtable and >>>> + * synchronized with field_plen. */ >>>> + if (!field_plen[j] || !ctx->trie->field) { >>>> + continue; >>>> + } >>>> + >>>> + uint8_t be32ofs = ctx->trie->field->flow_be32ofs; >>> >>> What will prevent 'field' being cleared between above two lines and produce >>> a NULL pointer dereference here or inside trie_lookup()? >> >> >> Main thread calls ovsrcu_synchronize() before it sets cls->tries[i]->field = >> NULL. >> Here ovsrcu_synchronize() waits for other readers(revalidator/handler thread) >> of rcu protected pointers going through RCU quiescent state. >> >> So here, main thread who called ovsrcu_synchronize() waits for >> revalidator/handler thread >> going through quiescent state. It means 'be32ofs = >> ctx->trie->field->flow_be32ofs;' >> is executed before main thread executes 'cls->tries[i]->field = NULL;’. > > It's correct that ovsrcu_synchronize() waits for other threads to go through > RCU > quiescent state, but it doesn't work as a thread barrier. Threads are free to > run, noone stops them and they could be at any point of their execution flow. > > And at the moment main thread sets field to NULL it's immediately visible to > all > threads in common case. And if one of the readers was right between above two > lines it will crash.
The code checks "!field_plen[j]” (RCU protected subtable->plen[j]) before it checks !ctx->trie->field. It means that reader threads do not see NULL field before main thread completes calling osvrcu_synchronize(). > >> >>> >>> trie->field is not protected by anything and not even an atomic variable. >>> Since it always points to some static memory objects it doesn't need to >>> be RCU protected, but it should be an atomic variable and it must be checked >>> after any read. In fact it might be made rcu protected just to make things >>> easier and more accurate. >>> >> >> I also had thought that protecting trie->field by RCU is a good idea, but >> found the >> following race condition: >> >> [main thread] [revalidator/handler thread] >> ======================================================================== >> ovsrcu_set(&trie->field, NULL); >> ovsrcu_synchronize(); >> Quiecense State; >> for (int i = 0; i < cls->n_tries; i++) >> /* field == NULL */ >> field = ovsrcu_get(struct mf_field *, >> &trie->field); >> be32ofs = field->flow_be32ofs; > > If fields is an RCU protected variable, you must check the value after each > ovsrcu_get() operation. I see. Now I’ve come to think that my initial approach is not easy to understand and fragile through discussion with you. I’ll rewrite my patch to make mf_field RCU protected. Please let me know if you have any other good idea. Thanks Eiichi _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
