[Open-FCoE] [PATCH 2/2] libfc: fixed some more possible fc_exch_reset() races

Mike Christie michaelc at cs.wisc.edu
Wed Sep 10 17:56:31 UTC 2008


Dev, Vasu wrote:
> Thanks Mike for your comments and sorry for late response since I took
> yesterday off.

No problem. My comments were late.

> 
>> -----Original Message-----
>> From: Mike Christie [mailto:michaelc at cs.wisc.edu]
>> Sent: Tuesday, September 09, 2008 8:44 AM
>> To: Dev, Vasu
>> Cc: devel at open-fcoe.org
>> Subject: Re: [Open-FCoE] [PATCH 2/2] libfc: fixed some more possible
>> fc_exch_reset() races
>>
>> Vasu Dev wrote:
>>>  /*
>>> @@ -553,6 +553,7 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr
> *mp,
>> u16 xid)
>>>  	fc_seq_alloc(ep, ep->seq_id++);
>>>  	mp->total_exches++;
>>>  	spin_unlock_bh(&mp->em_lock);
>>> +	fc_exch_hold(ep);	/* hold for exch in mp */
>>>
>>>  	/*
>>>  	 *  update exchange
>>> @@ -567,7 +568,12 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr
>> *mp, u16 xid)
>>>  	spin_lock_init(&ep->ex_lock);
>>>  	setup_timer(&ep->ex_timer, fc_exch_timeout, (unsigned long)ep);
>>>
>>> -	fc_exch_hold(ep);	/* hold for caller */
>>> +	/*
>>> +	 * Hold exch lock for caller to prevent
>>> +	 * fc_exch_reset() from releasing exch
>>> +	 * while caller is still working on exch.
>>> +	 */
>>> +	spin_lock_bh(&ep->ex_lock);
>>
>> I think we need to move the fc_exch_hold and ep initialization (code
>> after the "update exchange" comment in fc_exch_alloc to before where
> add
>> the ep to the mp->ex_list.
>>
>>
>> I am thinking about if fc_exch_mgr_reset runs right after fc_exch_alloc
>> does this:
>>
>>         mp->exches[xid - min_xid] = ep;
>>         list_add_tail(&ep->ex_list, &mp->ex_list);
>>         fc_seq_alloc(ep, ep->seq_id++);
>>         mp->total_exches++;
>>         spin_unlock_bh(&mp->em_lock);
>>
>>
>> fc_exch_mgr_reset will see the ep in the ex_list and call
> fc_exch_reset,
>> but the ep is not fully setup at this time, so when fc_exch_reset tries
>> to grab the ex_lock we should get an error about using a spin lock that
>> is not inited.
>>
> 
> Yes this is an issue and I also noticed this issue the same day this
> patch went out to this mail list, so I fixed this issue in ver-2 of this
> patch on the same day in this email
> http://www.open-fcoe.org/pipermail/devel/2008-September/000700.html .
> 
> I did report ver-2 of this patch invalidating this patch in this email
> http://www.open-fcoe.org/pipermail/devel/2008-September/000701.html. I
> fixed this issue in ver-2 by initializing and then grabbing ex_lock
> first before adding ep to ex_list/mp. This would ensure fc_exch_reset()
> to gate on ex_lock if fc_exch_reset() occurs once mp lock dropped in
> fc_exch_alloc().
>  
> Sorry for the thrash by two versions of this patch on the same day.

No problem. My fault. I thought I saw you resend a patch but I must have 
deleted ver2 instead of ver1.

The refcounting and lockinit in ver2 looks good.

> 
>> Do we also need to add a check for FC_EX_RST_CLEANUP in fc_exch_alloc
>> after we have grabbed the ex_lock in case after fc_exch_alloc dropped
>> the em_lock, fc_exch_mgr_reset had grabbed the em_lock, seen the ep in
>> the ex_list and called fc_exch_reset on it?
> 
> This check is not required after ver-2 of this patch since with ver-2
> the ex_lock is grabbed first and that would gate subsequent
> fc_exch_reset() on ex_lock once em_lock is dropped.
> 

See that too. Thanks.



More information about the devel mailing list