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

Jarno Rajahalme jrajahalme at nicira.com
Tue Aug 11 18:15:13 UTC 2015


> On Aug 11, 2015, at 10:42 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> 
> 
>> 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.
> 

I missed the fact that handle_missed_revalidation() in here also takes the ukey lock.

  Jarno

> 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