classifier: Fix race condition leading to NULL dereference.

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@ovn.org>

diff --git a/lib/classifier-private.h b/lib/classifier-private.h
index 0f8ad1e..babb64f 100644 (file)
--- a/lib/classifier-private.h
+++ b/lib/classifier-private.h
@@ -85,6 +85,19 @@ struct cls_match {
     /* 'flow' must be the last field. */
 };
 
+/* Utilities for accessing the 'cls_match' member of struct cls_rule. */
+static inline struct cls_match *
+get_cls_match_protected(const struct cls_rule *rule)
+{
+    return ovsrcu_get_protected(struct cls_match *, &rule->cls_match);
+}
+
+static inline struct cls_match *
+get_cls_match(const struct cls_rule *rule)
+{
+    return ovsrcu_get(struct cls_match *, &rule->cls_match);
+}
+
 /* Must be RCU postponed. */
 void cls_match_free_cb(struct cls_match *);
 

diff --git a/lib/classifier.c b/lib/classifier.c
index b30ae66..19e26b4 100644 (file)
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -172,7 +172,7 @@ cls_rule_init__(struct cls_rule *rule, unsigned int priority)
 {
     rculist_init(&rule->node);
     *CONST_CAST(int *, &rule->priority) = priority;
-    rule->cls_match = NULL;
+    ovsrcu_init(&rule->cls_match, NULL);
 }
 
 /* Initializes 'rule' to match packets specified by 'match' at the given
@@ -230,7 +230,8 @@ void
 cls_rule_destroy(struct cls_rule *rule)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
-    ovs_assert(!rule->cls_match);   /* Must not be in a classifier. */
+    /* Must not be in a classifier. */
+    ovs_assert(!get_cls_match_protected(rule));
 
     /* Check that the rule has been properly removed from the classifier. */
     ovs_assert(rule->node.prev == RCULIST_POISON
@@ -240,11 +241,12 @@ cls_rule_destroy(struct cls_rule *rule)
     minimatch_destroy(CONST_CAST(struct minimatch *, &rule->match));
 }
 
+/* This may only be called by the exclusive writer. */
 void
 cls_rule_set_conjunctions(struct cls_rule *cr,
                           const struct cls_conjunction *conj, size_t n)
 {
-    struct cls_match *match = cr->cls_match;
+    struct cls_match *match = get_cls_match_protected(cr);
     struct cls_conjunction_set *old
         = ovsrcu_get_protected(struct cls_conjunction_set *, &match->conj_set);
     struct cls_conjunction *old_conj = old ? old->conj : NULL;
@@ -285,23 +287,28 @@ cls_rule_is_catchall(const struct cls_rule *rule)
 /* Makes 'rule' invisible in 'remove_version'.  Once that version is used in
  * lookups, the caller should remove 'rule' via ovsrcu_postpone().
  *
- * 'rule' must be in a classifier. */
+ * 'rule' must be in a classifier.
+ * This may only be called by the exclusive writer. */
 void
 cls_rule_make_invisible_in_version(const struct cls_rule *rule,
                                    cls_version_t remove_version)
 {
-    ovs_assert(remove_version >= rule->cls_match->add_version);
+    struct cls_match *cls_match = get_cls_match_protected(rule);
+
+    ovs_assert(remove_version >= cls_match->add_version);
 
-    cls_match_set_remove_version(rule->cls_match, remove_version);
+    cls_match_set_remove_version(cls_match, remove_version);
 }
 
 /* This undoes the change made by cls_rule_make_invisible_in_version().
  *
- * 'rule' must be in a classifier. */
+ * 'rule' must be in a classifier.
+ * This may only be called by the exclusive writer. */
 void
 cls_rule_restore_visibility(const struct cls_rule *rule)
 {
-    cls_match_set_remove_version(rule->cls_match, CLS_NOT_REMOVED_VERSION);
+    cls_match_set_remove_version(get_cls_match_protected(rule),
+                                 CLS_NOT_REMOVED_VERSION);
 }
 
 /* Return true if 'rule' is visible in 'version'.
@@ -310,7 +317,9 @@ cls_rule_restore_visibility(const struct cls_rule *rule)
 bool
 cls_rule_visible_in_version(const struct cls_rule *rule, cls_version_t version)
 {
-    return cls_match_visible_in_version(rule->cls_match, version);
+    struct cls_match *cls_match = get_cls_match(rule);
+
+    return cls_match && cls_match_visible_in_version(cls_match, version);
 }
 \f
 /* Initializes 'cls' as a classifier that initially contains no classification
@@ -540,8 +549,7 @@ classifier_replace(struct classifier *cls, const struct cls_rule *rule,
 
     /* 'new' is initially invisible to lookups. */
     new = cls_match_alloc(rule, version, conjs, n_conjs);
-
-    CONST_CAST(struct cls_rule *, rule)->cls_match = new;
+    ovsrcu_set(&CONST_CAST(struct cls_rule *, rule)->cls_match, new);
 
     subtable = find_subtable(cls, rule->match.mask);
     if (!subtable) {
@@ -636,8 +644,9 @@ classifier_replace(struct classifier *cls, const struct cls_rule *rule,
                     ovsrcu_postpone(free, conj_set);
                 }
 
+                ovsrcu_set(&old->cls_match, NULL); /* Marks old rule as removed
+                                                    * from the classifier. */
                 ovsrcu_postpone(cls_match_free_cb, iter);
-                old->cls_match = NULL;
 
                 /* No change in subtable's max priority or max count. */
 
@@ -728,12 +737,12 @@ classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
     size_t n_rules;
     unsigned int i;
 
-    rule = cls_rule->cls_match;
+    rule = get_cls_match_protected(cls_rule);
     if (!rule) {
         return NULL;
     }
     /* Mark as removed. */
-    CONST_CAST(struct cls_rule *, cls_rule)->cls_match = NULL;
+    ovsrcu_set(&CONST_CAST(struct cls_rule *, cls_rule)->cls_match, NULL);
 
     /* Remove 'cls_rule' from the subtable's rules list. */
     rculist_remove(CONST_CAST(struct rculist *, &cls_rule->node));
@@ -1261,7 +1270,7 @@ classifier_rule_overlaps(const struct classifier *cls,
             if (rule->priority == target->priority
                 && miniflow_equal_in_minimask(target->match.flow,
                                               rule->match.flow, &m.mask)
-                && cls_match_visible_in_version(rule->cls_match, version)) {
+                && cls_rule_visible_in_version(rule, version)) {
                 return true;
             }
         }
@@ -1318,7 +1327,7 @@ rule_matches(const struct cls_rule *rule, const struct cls_rule *target,
              cls_version_t version)
 {
     /* Rule may only match a target if it is visible in target's version. */
-    return cls_match_visible_in_version(rule->cls_match, version)
+    return cls_rule_visible_in_version(rule, version)
         && (!target || miniflow_equal_in_minimask(rule->match.flow,
                                                   target->match.flow,
                                                   target->match.mask));

diff --git a/lib/classifier.h b/lib/classifier.h
index 57a9593..2af5643 100644 (file)
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -357,7 +357,8 @@ struct cls_conjunction {
 struct cls_rule {
     struct rculist node;          /* In struct cls_subtable 'rules_list'. */
     const int priority;           /* Larger numbers are higher priorities. */
-    struct cls_match *cls_match;  /* NULL if not in a classifier. */
+    OVSRCU_TYPE(struct cls_match *) cls_match;  /* NULL if not in a
+                                                 * classifier. */
     const struct minimatch match; /* Matching rule. */
 };
 

diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index 0242f6a..c74c440 100644 (file)
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -450,8 +450,7 @@ compare_classifiers(struct classifier *cls, size_t n_invisible_rules,
             assert(tr0->aux == tr1->aux);
 
             /* Make sure the rule should have been visible. */
-            assert(cr0->cls_match);
-            assert(cls_match_visible_in_version(cr0->cls_match, version));
+            assert(cls_rule_visible_in_version(cr0, version));
         }
         cr2 = classifier_lookup(cls, version, &flow, NULL);
         assert(cr2 == cr0);
@@ -611,16 +610,18 @@ check_tables(const struct classifier *cls, int n_tables, int n_rules,
                 found_rule = classifier_find_rule_exactly(cls, rule->cls_rule,
                                                           rule_version);
                 if (found_rule && found_rule != rule->cls_rule) {
+                    struct cls_match *cls_match;
+                    cls_match = get_cls_match_protected(found_rule);
 
                     assert(found_rule->priority == rule->priority);
 
                     /* Found rule may not have a lower version. */
-                    assert(found_rule->cls_match->add_version >= rule_version);
+                    assert(cls_match->add_version >= rule_version);
 
                     /* This rule must not be visible in the found rule's
                      * version. */
                     assert(!cls_match_visible_in_version(
-                               rule, found_rule->cls_match->add_version));
+                               rule, cls_match->add_version));
                 }
 
                 if (rule->priority == prev_priority) {
@@ -1030,10 +1031,12 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
                              version);
 
                 if (versioned && removable_rule) {
+                    struct cls_match *cls_match =
+                        get_cls_match_protected(removable_rule);
+
                     /* Removable rule is no longer visible. */
-                    assert(removable_rule->cls_match);
-                    assert(!cls_match_visible_in_version(
-                               removable_rule->cls_match, version));
+                    assert(cls_match);
+                    assert(!cls_match_visible_in_version(cls_match, version));
                     classifier_remove(&cls, removable_rule);
                     n_invisible_rules--;
                 }