[ovs-dev] [PATCH v2 1/2] upcall: Fix minor race when deleting ukeys.

Jarno Rajahalme jrajahalme at nicira.com
Tue Aug 11 17:42:51 UTC 2015


> On Aug 10, 2015, at 6:46 PM, Ethan J. Jackson <ethan at nicira.com> wrote:
> 
> From: Ethan Jackson <ethan at nicira.com>
> 
> Since revalidator_sweep() doesn't hold the ukey mutex for each full
> loop iteration, it's theoretically possible that two threads may
> call ukey_delete() on the same ukey.  If this happens, they both will
> attempt to remove the ukey from he cmap, causing the loser of the race
> to fail.
> 
> Signed-off-by: Ethan J. Jackson <ethan at nicira.com>
> ---
> ofproto/ofproto-dpif-upcall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 0f2e186..fddb535 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2076,7 +2076,6 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>             flow_exists = ukey->flow_exists;
>             seq_mismatch = (ukey->dump_seq != dump_seq
>                             && ukey->reval_seq != reval_seq);
> -            ovs_mutex_unlock(&ukey->mutex);
> 
>             if (flow_exists
>                 && (purge

There is a call to push_ukey_ops() between these blocks. It calls push_ukey_ops__(), which will take a lock on the ukeys, including the last one added and still locked, so this would result in double locking.

This could be resolved by moving the “ if (n_ops == REVALIDATE_MAX_BATCH)” block after the (moved) ukey unlock, similarly to the remainder ops handling at the end. However, I’m not sure if this would defeat the purpose of this patch.

Also, it seems that generally the umap lock is taken first and ukey locks after, so there is a possibility of a deadlock if the locks are taken in the reverse order, like in this patch.

It would be really great if we could employ clang thread safety analysis more than we do in this file currently, but I’m not sure if that is possible.

  Jarno

> @@ -2095,6 +2094,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>                 ukey_delete(umap, ukey);
>                 ovs_mutex_unlock(&umap->mutex);
>             }
> +            ovs_mutex_unlock(&ukey->mutex);
>         }
> 
>         if (n_ops) {
> -- 
> 2.1.0
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list