rculist: Remove postponed poisoning.

Postponed 'next' member poisoning was based on the faulty assumption
that postponed functions would be called in the order they were
postponed.  This assumption holds only for the functions postponed by
any single thread.  When functions are postponed by different
threads, there are no guarantees of the order in which the functions
may be called, or timing between those calls after the next grace
period has passed.

Given this, the postponed poisoning could have executed after
postponed destruction of the object containing the rculist element.

This bug was revealed after the memory leaks on rule deletion were
recently fixed.

This patch removes the postponed 'next' member poisoning and adds
documentation describing the ordering limitations in OVS RCU.

Alex Wang dug out the root cause of the resulting crashes, thanks!

Signed-off-by: Jarno Rajahalme <[email protected]>
Acked-by: Alex Wang <[email protected]>

lib/automake.mk

@@ -197,7 +197,6 @@ lib_libopenvswitch_la_SOURCES = \
 	lib/random.h \
 	lib/rconn.c \
 	lib/rconn.h \
-	lib/rculist.c \
 	lib/rculist.h \
 	lib/reconnect.c \
 	lib/reconnect.h \

lib/classifier.c

@@ -241,14 +241,14 @@ cls_rule_move(struct cls_rule *dst, struct cls_rule *src)
  * ('rule' must not currently be in a classifier.) */
 void
 cls_rule_destroy(struct cls_rule *rule)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     ovs_assert(!rule->cls_match);   /* Must not be in a classifier. */
 
-    /* Check that the rule has been properly removed from the classifier and
-     * that the destruction only happens after the RCU grace period, or that
-     * the rule was never inserted to the classifier in the first place. */
-    ovs_assert(rculist_next_protected(&rule->node) == RCULIST_POISON
+    /* Check that the rule has been properly removed from the classifier. */
+    ovs_assert(rule->node.prev == RCULIST_POISON
                || rculist_is_empty(&rule->node));
+    rculist_poison__(&rule->node);   /* Poisons also the next pointer. */
 
     minimatch_destroy(CONST_CAST(struct minimatch *, &rule->match));
 }

lib/ovs-rcu.c

@@ -212,6 +212,19 @@ ovsrcu_synchronize(void)
 /* Registers 'function' to be called, passing 'aux' as argument, after the
  * next grace period.
  *
+ * The call is guaranteed to happen after the next time all participating
+ * threads have quiesced at least once, but there is no quarantee that all
+ * registered functions are called as early as possible, or that the functions
+ * registered by different threads would be called in the order the
+ * registrations took place.  In particular, even if two threads provably
+ * register a function each in a specific order, the functions may still be
+ * called in the opposite order, depending on the timing of when the threads
+ * call ovsrcu_quiesce(), how many functions they postpone, and when the
+ * ovs-rcu thread happens to grab the functions to be called.
+ *
+ * All functions registered by a single thread are guaranteed to execute in the
+ * registering order, however.
+ *
  * This function is more conveniently called through the ovsrcu_postpone()
  * macro, which provides a type-safe way to allow 'function''s parameter to be
  * any pointer type. */

lib/ovs-rcu.h

@@ -60,12 +60,25 @@
  *
  * When a quiescient state has occurred in every thread, we say that a "grace
  * period" has occurred.  Following a grace period, all of the callbacks
- * postponed before the start of the grace period may be invoked.  OVS takes
+ * postponed before the start of the grace period MAY be invoked.  OVS takes
  * care of this automatically through the RCU mechanism: while a process still
  * has only a single thread, it invokes the postponed callbacks directly from
  * ovsrcu_quiesce() and ovsrcu_quiesce_start(); after additional threads have
  * been created, it creates an extra helper thread to invoke callbacks.
  *
+ * Please note that while a postponed function call is guaranteed to happen
+ * after the next time all participating threads have quiesced at least once,
+ * there is no quarantee that all postponed functions are called as early as
+ * possible, or that the functions postponed by different threads would be
+ * called in the order the registrations took place.  In particular, even if
+ * two threads provably postpone a function each in a specific order, the
+ * postponed functions may still be called in the opposite order, depending on
+ * the timing of when the threads call ovsrcu_quiesce(), how many functions
+ * they postpone, and when the ovs-rcu thread happens to grab the functions to
+ * be called.
+ *
+ * All functions postponed by a single thread are guaranteed to execute in the
+ * order they were postponed, however.
  *
  * Use
  * ---

lib/rculist.h

@@ -38,10 +38,7 @@
  * - rculist_front() returns a const pointer to accommodate for an RCU reader.
  * - rculist_splice_hidden(): Spliced elements may not have been visible to
  *   RCU readers before the operation.
- * - rculist_poison(): Immediately poisons the 'prev' pointer, and schedules
- *   ovsrcu_postpone() to poison the 'next' pointer.  This issues a memory
- *   write operation to the list element, hopefully crashing the program if
- *   the list node was freed or re-used too early.
+ * - rculist_poison(): Only poisons the 'prev' pointer.
  *
  * The following functions are variations of the struct ovs_list functions with
  * similar names, but are now restricted to the writer use:
@@ -134,16 +131,26 @@ rculist_init(struct rculist *list)
 
 #define RCULIST_POISON (struct rculist *)(UINTPTR_MAX / 0xf * 0xc)
 
-void rculist_poison__(struct rculist *list);
-
 /* Initializes 'list' with pointers that will (probably) cause segfaults if
  * dereferenced and, better yet, show up clearly in a debugger. */
 static inline void
 rculist_poison(struct rculist *list)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     list->prev = RCULIST_POISON;
-    ovsrcu_postpone(rculist_poison__, list);
+}
+
+/* Initializes 'list' with pointers that will (probably) cause segfaults if
+ * dereferenced and, better yet, show up clearly in a debugger.
+ *
+ * This variant poisons also the next pointer, so this may not be called if
+ * this list element is still visible to RCU readers. */
+static inline void
+rculist_poison__(struct rculist *list)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
+{
+    rculist_poison(list);
+    ovsrcu_set_hidden(&list->next, RCULIST_POISON);
 }
 
 /* rculist insertion. */
@@ -217,10 +224,7 @@ rculist_replace(struct rculist *element, struct rculist *position)
     position_next->prev = element;
     element->prev = position->prev;
     ovsrcu_set(&element->prev->next, element);
-
-#ifndef NDEBUG
-    rculist_poison(position); /* XXX: Some overhead due to ovsrcu_postpone() */
-#endif
+    rculist_poison(position);
 }
 
 /* Initializes 'dst' with the contents of 'src', compensating for moving it
@@ -244,10 +248,7 @@ rculist_move(struct rculist *dst, struct rculist *src)
     } else {
         rculist_init(dst);
     }
-
-#ifndef NDEBUG
-    rculist_poison(src); /* XXX: Some overhead due to ovsrcu_postpone() */
-#endif
+    rculist_poison(src);
 }
 
 /* Removes 'elem' from its list and returns the element that followed it.
@@ -268,10 +269,7 @@ rculist_remove(struct rculist *elem)
 
     elem_next->prev = elem->prev;
     ovsrcu_set(&elem->prev->next, elem_next);
-
-#ifndef NDEBUG
-    rculist_poison(elem); /* XXX: Some overhead due to ovsrcu_postpone() */
-#endif
+    rculist_poison(elem);
     return elem_next;
 }