Thanks Mike for your comments and sorry for late response since I took yesterday off.
>-----Original Message----- >From: Mike Christie [mailto:[EMAIL PROTECTED] >Sent: Tuesday, September 09, 2008 8:44 AM >To: Dev, Vasu >Cc: [email protected] >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. _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
