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:[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. 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. _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
