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 3e5bc7a..7ec7e8d 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 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.
@@ -216,6 +216,23 @@ udpif_recv_set(struct udpif *udpif, size_t n_handlers, bool enable)
     }
 }
 
+/* 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;
+    if (n_handlers) {
+        udpif_recv_set(udpif, 0, false);
+        udpif_recv_set(udpif, n_handlers, true);
+    }
+}
+
 void
 udpif_wait(struct udpif *udpif)
 {

diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
index 57d462d..c610484 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.
@@ -34,6 +34,7 @@ struct dpif_backer;
 
 struct udpif *udpif_create(struct dpif_backer *, struct dpif *);
 void udpif_recv_set(struct udpif *, size_t n_workers, bool enable);
+void udpif_synchronize(struct udpif *);
 void udpif_destroy(struct udpif *);
 
 void udpif_wait(struct udpif *);

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3fce1ef..cf421ae 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1407,9 +1407,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);