On Thu, Feb 02, 2012 at 11:47:50AM -0800, Bhanu Prakash Gollapudi wrote:
> On 2/2/2012 7:50 AM, Neil Horman wrote:
> >fc_port->ema_list is being accessed in multiple parallel contexts with no
> >locking around it leading to potential corruption. Whats more the
> >fc_find_ema
> >function was manually accessing ema_list.prev without checking for a
> >list_empty
> >condition. The ema_list could easily be emtpy if an transation came in to
> >the
> >foce stack with an UNKNOWN xid prior to our adding a ema anchor to the list
> >(dev_add_pack gets called before said add occurs).
> >
> >Fix it by adding an ema_lock to list accesses. Make it an rcu lock so we
> >don't
> >get Warnings when we try to tx a frame with a spinlock held
> >
> >Signed-off-by: Neil Horman<nhor...@tuxdriver.com>
> >CC: kiran.pa...@intel.com
> >CC: bprak...@broadcom.com
> >CC: robert.w.l...@intel.com
> >---
> > drivers/scsi/libfc/fc_exch.c | 102
> > +++++++++++++++++++++++++++++++-----------
> > include/scsi/libfc.h | 2 +
> > 2 files changed, 77 insertions(+), 27 deletions(-)
> >
> >diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
> >index 630291f..9c63464 100644
> >--- a/drivers/scsi/libfc/fc_exch.c
> >+++ b/drivers/scsi/libfc/fc_exch.c
> >@@ -130,6 +130,7 @@ struct fc_exch_mgr_anchor {
> > struct list_head ema_list;
> > struct fc_exch_mgr *mp;
> > bool (*match)(struct fc_frame *);
> >+ struct rcu_head rcu;
> > };
> >
> > static void fc_exch_rrq(struct fc_exch *);
> >@@ -402,9 +403,10 @@ static void fc_exch_delete(struct fc_exch *ep)
> > {
> > struct fc_exch_pool *pool;
> > u16 index;
> >+ unsigned long flags;
> >
> > pool = ep->pool;
> >- spin_lock_bh(&pool->lock);
> >+ spin_lock_irqsave(&pool->lock, flags);
>
> There is no contention between hard IRQs, so why do we need
> spin_lock_irqsave()? spin_lock_bh() should be good enough as the
> contention is only between softirq and process context.
>
Dang it, the pool_lock changes are pointless, they weren't meant to be in here,
just the ema_lock additions. My git tree commits must be dirty, I'll repost
shortly.
> If we can use spin_lock_bh(), second patch can be avoided.
>
Thats incorrect. The second patch deals with managing the ex_lock,
not this new ema_lock, and it deals with dropping it prior to calling
fcoe_seq_send. Specifically we have to drop it because transmitting a frame
with bottom halves disabled generates a warning.
>
_______________________________________________
devel mailing list
devel@open-fcoe.org
https://lists.open-fcoe.org/mailman/listinfo/devel
