From: Ben Pfaff <blp@nicira.com>
Date: Tue, 25 Feb 2014 16:01:01 +0000 (-0800)
Subject: ofproto-dpif: Complete all packet translations before freeing an ofproto.
X-Git-Tag: sliver-openvswitch-2.2.90-1~9^2~46
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;ds=sidebyside;h=3f142f59f2859ef5c2bc124405ab4d683d3f416b;p=sliver-openvswitch.git

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 cad131088..53295aade 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -412,6 +412,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 d73ae4c9a..9eeee5b90 100644
--- 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 64e27473a..c59711476 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1182,9 +1182,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);