From dd2e44f835fac8c2df99f84c54250c3ca981f2f5 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 25 Feb 2014 08:01:01 -0800 Subject: [PATCH] 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 Acked-by: Alex Wang --- ofproto/ofproto-dpif-upcall.c | 19 ++++++++++++++++++- ofproto/ofproto-dpif-upcall.h | 3 ++- ofproto/ofproto-dpif.c | 6 +++--- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 3e5bc7ae9..7ec7e8d49 100644 --- 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 57d462ded..c6104844c 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. @@ -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 3fce1ef7b..cf421ae18 100644 --- 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); -- 2.20.1