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

[Ilya Maximets]

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.tsukata at 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.