ofproto-dpif: Complete all packet translations before freeing an ofproto.

The following scenario can occur:

   1. Handler thread grabs a pointer to an ofproto in handle_upcalls().

   2. Main thread removes ofproto and destroys it in destruct().

   3. Handler thread uses pointer to ofproto and accesses freed memory.
      BOOM!

Each individual step above happens under the xlate_rwlock, but the ofproto
pointer is retained from step 1 to step 3, hence the problem.  This commit
fixes the problem by ensuring that after an ofproto is removed but before
it is destroyed, all packet translations get pushed all the way through
the upcall handler pipeline.  (No new packet translations can get a pointer
to the removed ofproto.)

Bug #1200351.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 9fc2e2d..fbc9ed8 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -405,6 +405,22 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers,
     }
 }
 
+/* Waits for all ongoing upcall translations to complete.  This ensures that
+ * there are no transient references to any removed ofprotos (or other
+ * objects).  In particular, this should be called after an ofproto is removed
+ * (e.g. via xlate_remove_ofproto()) but before it is destroyed. */
+void
+udpif_synchronize(struct udpif *udpif)
+{
+    /* This is stronger than necessary.  It would be sufficient to ensure
+     * (somehow) that each handler and revalidator thread had passed through
+     * its main loop once. */
+    size_t n_handlers = udpif->n_handlers;
+    size_t n_revalidators = udpif->n_revalidators;
+    udpif_set_threads(udpif, 0, 0);
+    udpif_set_threads(udpif, n_handlers, n_revalidators);
+}
+
 /* Notifies 'udpif' that something changed which may render previous
  * xlate_actions() results invalid. */
 void

diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
index d73ae4c..9eeee5b 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.h
+++ b/ofproto/ofproto-dpif-upcall.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2013 Nicira, Inc.
+/* Copyright (c) 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,6 +29,7 @@ struct simap;
 struct udpif *udpif_create(struct dpif_backer *, struct dpif *);
 void udpif_set_threads(struct udpif *, size_t n_handlers,
                        size_t n_revalidators);
+void udpif_synchronize(struct udpif *);
 void udpif_destroy(struct udpif *);
 void udpif_revalidate(struct udpif *);
 void udpif_get_memory_usage(struct udpif *, struct simap *usage);

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3778ca2..ca7c0b1 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1035,9 +1035,9 @@ destruct(struct ofproto *ofproto_)
     xlate_remove_ofproto(ofproto);
     ovs_rwlock_unlock(&xlate_rwlock);
 
-    /* Discard any flow_miss_batches queued up for 'ofproto', avoiding a
-     * use-after-free error. */
-    udpif_revalidate(ofproto->backer->udpif);
+    /* Ensure that the upcall processing threads have no remaining references
+     * to the ofproto or anything in it. */
+    udpif_synchronize(ofproto->backer->udpif);
 
     hmap_remove(&all_ofproto_dpifs, &ofproto->all_ofproto_dpifs_node);