[ovs-dev] [PATCH] ofproto-dpif-upcall: Avoid double-delete of ukeys.

[Ben Pfaff]

On Wed, Jan 06, 2016 at 02:50:36PM -0800, Joe Stringer wrote:
> On 5 January 2016 at 16:24, Ben Pfaff <blp at ovn.org> wrote:
> > revalidate_sweep__() has two cases where it calls ukey_delete() to
> > remove a ukey from the umap via cmap_remove().  The first case is a direct
> > call to ukey_delete(), when !flow_exists.  The second case is an indirect
> > call via push_ukey_ops(), when result != UKEY_KEEP.  If both of these
> > conditions are simultaneously true, however, the code would call
> > ukey_delete() twice, causing an assertion failure in the second call.  This
> > commit fixes the problem by eliminating one of the calls.
> >
> > Reported-by: Keith Holleman <keith.holleman at gmail.com>
> > Reported-at: http://openvswitch.org/pipermail/discuss/2015-December/019772.html
> > CC: Joe Stringer <joe at ovn.org>
> > VMware-BZ: #1579057
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> >
> > ---
> >  ofproto/ofproto-dpif-upcall.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> > index cd8a9f0..ca25701 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -1,4 +1,4 @@
> > -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> > +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
> >   *
> >   * Licensed under the Apache License, Version 2.0 (the "License");
> >   * you may not use this file except in compliance with the License.
> > @@ -2274,7 +2274,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
> >                  n_ops = 0;
> >              }
> >
> > -            if (!flow_exists) {
> > +            if (result == UKEY_KEEP && !flow_exists) {
> >                  ovs_mutex_lock(&umap->mutex);
> >                  ukey_delete(umap, ukey);
> >                  ovs_mutex_unlock(&umap->mutex);
> 
> Thanks for finding this.
> 
> In the common case, I would expect the flow to be deleted during the
> revalidator "dump" (mark) phase. With the current code, and with this
> approach, we may end up attempting to delete the same flows multiple
> times. I suggest the below instead:
> 
> @@ -2262,7 +2264,7 @@ revalidator_sweep__(struct revalidator
> *revalidator, bool purge)
>                  result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
>                                           reval_seq, &recircs);
>              }
> -            if (result != UKEY_KEEP) {
> +            if (flow_exists && result != UKEY_KEEP) {
>                  /* Takes ownership of 'recircs'. */
>                  reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
>                                &odp_actions);

Thanks!  I applied this change and pushed it to master and branch-2.5.

> I notice a couple of other minor issues as well while looking at this code:
> - We may currently revalidate ukeys in revalidator_sweep() whenever
> the dump_seq/reval_seq mismatch, even if the ukey's corresponding flow
> is already deleted. This is unnecessary: If we have previously deleted
> the flow, we can just delete the ukey.
> - If there is a seq mismatch in dump_seq or reval_seq, and
> revalidate_ukey() returns UKEY_MODIFY, the flow will be modified and
> the ukey deleted. I believe that this will not clear the datapath
> stats, so next time the flow is dumped, all previous stats for the
> flow will be double-attributed. This seems fairly unlikely as it
> should only happen if the seqs change in between dump phase and
> revalidation phase, or if the flow is not dumped by the kernel (as
> well as only if revalidation requires modification of the flow - ie
> actions changed but flows did not).
> 
> I will send patches for these latter issues.

Thanks!