Thanks, applied to master, branch-2.0, branch-2.1.
On Mon, Feb 24, 2014 at 12:25:41PM -0800, Alex Wang wrote: > 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
