[ovs-dev] [PATCH] ofproto-dpif-upcall: Fix additional use-after-free in revalidate().

[Ben Pfaff]

I forgot to mention that this is for branch-2.3, since this is already
fixed on master.

On Wed, May 21, 2014 at 02:33:51PM -0700, Ben Pfaff wrote:
> Commit 1340ce0c175 (ofproto-dpif-upcall: Avoid use-after-free in
> revalidate() corner cases.) fixed one use-after-free error in revalidate(),
> but missed one more subtle case, in which dpif_linux_flow_dump_next()
> attempts to retrieve actions for a flow that didn't have them in the main
> dump result.  If retrieving those actions fails,
> dpif_linux_flow_dump_next() goes on to the next flow, and as part of that
> overwrites the old dumped flows in its buffer.  This is a problem because
> dpif_linux_flow_dump_next_may_destroy_keys() would have indicated that
> the old dumped flows would not have been destroyed, which means that the
> data the caller relied on has gone away.  In the worst case, this causes
> a segfault and core dump.
> 
> The best way to fix this problem is the refactoring that has already
> happened on master in commit ac64794acb85 (dpif: Refactor flow dumping
> interface to make better sense for batching.)  That is a fairly large
> change, and not yet well-tested, so it doesn't yet seem suitable for a
> stable branch.  For now, this commit simply turns off batching of the
> deletions that happen during revalidation.  If this proves in practice to
> hurt performance too much (e.g. in a TCP_CRR netperf when megaflows don't
> avoid per-microflow upcalls), we can always backport the refactoring.
> 
> Bug #1249988.
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/dpif.c                    |   15 +++------------
>  ofproto/ofproto-dpif-upcall.c |   30 ++++++++----------------------
>  2 files changed, 11 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 41b8eb7..677816f 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1065,18 +1065,9 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, void *state,
>      return !error;
>  }
>  
> -/* Determines whether the next call to 'dpif_flow_dump_next' for 'dump' and
> - * 'state' will modify or free the keys that it previously returned. 'state'
> - * must have been initialized by a call to 'dpif_flow_dump_state_init' for
> - * 'dump'.
> - *
> - * 'dpif' guarantees that data returned by flow_dump_next() will remain
> - * accessible and unchanging until the next call. This function provides a way
> - * for callers to determine whether that guarantee extends beyond the next
> - * call.
> - *
> - * Returns true if the next call to flow_dump_next() is expected to be
> - * destructive to previously returned keys for 'state', false otherwise. */
> +/* Returns true if the next call to flow_dump_next() is expected to be
> + * destructive to previously returned keys for 'state', false otherwise.
> + * The return value is only a heuristic; the caller must not rely on it. */
>  bool
>  dpif_flow_dump_next_may_destroy_keys(struct dpif_flow_dump *dump, void *state)
>  {
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 193f4cd..c8ec22c 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1434,16 +1434,13 @@ revalidate(struct revalidator *revalidator)
>  {
>      struct udpif *udpif = revalidator->udpif;
>  
> -    struct dump_op ops[REVALIDATE_MAX_BATCH];
>      const struct nlattr *key, *mask, *actions;
>      size_t key_len, mask_len, actions_len;
>      const struct dpif_flow_stats *stats;
>      long long int now;
>      unsigned int flow_limit;
> -    size_t n_ops;
>      void *state;
>  
> -    n_ops = 0;
>      now = time_msec();
>      atomic_read(&udpif->flow_limit, &flow_limit);
>  
> @@ -1451,7 +1448,7 @@ revalidate(struct revalidator *revalidator)
>      while (dpif_flow_dump_next(&udpif->dump, state, &key, &key_len, &mask,
>                                 &mask_len, &actions, &actions_len, &stats)) {
>          struct udpif_key *ukey;
> -        bool mark, may_destroy;
> +        bool mark;
>          long long int used, max_idle;
>          uint32_t hash;
>          size_t n_flows;
> @@ -1508,31 +1505,20 @@ revalidate(struct revalidator *revalidator)
>          }
>  
>          if (!mark) {
> -            dump_op_init(&ops[n_ops++], key, key_len, ukey);
> +            struct dump_op op;
> +
> +            dump_op_init(&op, key, key_len, ukey);
> +            push_dump_ops__(udpif, &op, 1);
>          }
>  
>      next:
> -        may_destroy = dpif_flow_dump_next_may_destroy_keys(&udpif->dump,
> -                                                           state);
> -
> -        /* Only update 'now' immediately before 'buffer' will be updated.
> +        /* Only update 'now' immediately before 'buffer' will be updated
> +         * (typically, at least, since this function only yields a heuristic).
>           * This gives us the current time relative to the time the datapath
>           * will write into 'stats'. */
> -        if (may_destroy) {
> +        if (dpif_flow_dump_next_may_destroy_keys(&udpif->dump, state)) {
>              now = time_msec();
>          }
> -
> -        /* Only do a dpif_operate when we've hit our maximum batch, or when our
> -         * memory is about to be clobbered by the next call to
> -         * dpif_flow_dump_next(). */
> -        if (n_ops == REVALIDATE_MAX_BATCH || (n_ops && may_destroy)) {
> -            push_dump_ops__(udpif, ops, n_ops);
> -            n_ops = 0;
> -        }
> -    }
> -
> -    if (n_ops) {
> -        push_dump_ops__(udpif, ops, n_ops);
>      }
>  
>      dpif_flow_dump_state_uninit(udpif->dpif, state);
> -- 
> 1.7.10.4
>