On 2/2/2012 12:30 PM, Neil Horman wrote:
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.
You're hitting the following warning right?
static inline void _local_bh_enable_ip(unsigned long ip)
{
WARN_ON_ONCE(in_irq() || irqs_disabled());
I think this is because of acquiring ex_lock using spin_lock_irqsave()
(one of the changes done in this patch). If it was spin_lock_bh(), you
should not have seen this. Isn't it?
_______________________________________________
devel mailing list
devel@open-fcoe.org
https://lists.open-fcoe.org/mailman/listinfo/devel
