Acked-by: Alex Wang <[email protected]>
On Mon, Feb 24, 2014 at 11:10 AM, Ben Pfaff <[email protected]> wrote: > 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 <[email protected]> > --- > ofproto/ofproto-dpif-upcall.c | 16 ++++++++++++++++ > ofproto/ofproto-dpif-upcall.h | 3 ++- > ofproto/ofproto-dpif.c | 6 +++--- > 3 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index cad1310..53295aa 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 d73ae4c..9eeee5b 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 64e2747..c597114 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); > > -- > 1.7.10.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
