[ovs-dev] [PATCH] ofproto-dpif: Complete all packet translations before freeing an ofproto.

[Alex Wang]

Acked-by: Alex Wang <alexw at nicira.com>



On Mon, Feb 24, 2014 at 11:10 AM, Ben Pfaff <blp at nicira.com> 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 <blp at nicira.com>
> ---
>  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
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140224/12ddcddd/attachment-0005.html>