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

Dev, Vasu vasu.dev at intel.com
Wed Sep 10 17:49:02 UTC 2008


Thanks Mike for your comments and sorry for late response since I took
yesterday off.

>-----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.

>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.



More information about the devel mailing list