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

Mike Christie michaelc at cs.wisc.edu
Tue Sep 9 15:43:58 UTC 2008


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.

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?



More information about the devel mailing list