Hello Ilya
> On May 28, 2020, at 7:28, Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 5/27/20 4:13 AM, Eiichi Tsukata wrote:
>> Currently classifier tries and n_tries can be updated not atomically,
>> there is a race condition which can lead to NULL dereference.
>> The race can happen when main thread updates a classifier tries and
>> n_tries in classifier_set_prefix_fields() and at the same time revalidator
>> or handler thread try to lookup them in classifier_lookup__(). Such race
>> can be triggered when user changes prefixes of flow_table.
>>
>> Race(user changes flow_table prefixes: ip_dst,ip_src => none):
>>
>> [main thread] [revalidator/handler thread]
>> ===========================================================
>> /* cls->n_tries == 2 */
>> for (int i = 0; i < cls->n_tries; i++) {
>> trie_init(cls, i, NULL);
>> /* n_tries == 0 */
>> cls->n_tries = n_tries;
>> /* cls->tries[i]->feild is NULL */
>> trie_ctx_init(&trie_ctx[i],&cls->tries[i]);
>> /* trie->field is NULL */
>> ctx->be32ofs = trie->field->flow_be32ofs;
>>
>> To prevent the race, instead of re-introducing internal mutex
>> implemented in the commit fccd7c092e09 ("classifier: Remove internal
>> mutex."), this patch makes trie field RCU protected and checks it after
>> read.
>>
>> Fixes: fccd7c092e09 ("classifier: Remove internal mutex.")
>> Signed-off-by: Eiichi Tsukata <eiichi.tsuk...@nutanix.com>
>> ---
>
> Patch looks good in general, but I'd suggest following incremental change:
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 00e87012c..f2c3497c2 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -1577,16 +1577,17 @@ 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? */
> + /* Is the trie field relevant for this subtable? */
> if (field_plen[j]) {
> + struct trie_ctx *ctx = &trie_ctx[j];
> const struct mf_field *ctx_field
> - = ovsrcu_get(struct mf_field *, &trie_ctx[j].trie->field);
> + = ovsrcu_get(struct mf_field *, &ctx->trie->field);
> +
> + /* Is the trie field within the current range of fields? */
> if (!ctx_field
> || !flowmap_is_set(&range_map, ctx_field->flow_be32ofs / 2)) {
> continue;
> }
> - struct trie_ctx *ctx = &trie_ctx[j];
>
> /* On-demand trie lookup. */
> if (!ctx->lookup_done) {
> ---
>
> What do you think?
>
> If you're ok with it, I could just squash above diff into your patch before
> applying.
>
> Best regards, Ilya Maximets.
Thanks for your suggestion. I think your diff looks pretty nice and makes codes
easier to understand.
I’m glad if you could squash it.
Best regards,
Eiichi
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
