> -----Original Message----- > From: iari@xxxxxx [mailto:iari@xxxxxx] > Sent: Monday, October 17, 2016 1:00 PM > To: Jiri Kosina > Cc: Kashyap Desai; Sumit Saxena; Uday Lingala; James E.J. Bottomley; Martin K. > Petersen; megaraidlinux.pdl@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; Iago > Abal > Subject: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to > avoid double lock > > From: Iago Abal <mail@xxxxxxxxxxx> > > The EBA code analyzer (https://github.com/models-team/eba) reported the > following double lock: > > 1. In function `megaraid_reset_handler' at 2571; > 2. take `&adapter->pend_list_lock' for the first time at 2602: > > // FIRST > spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags); > > 3. enter the `list_for_each_entry_safe' loop at 2603; > 4. call `megaraid_mbox_mm_done' at 2616; > 5. call `megaraid_mbox_runpendq' at 3782; > 6. take `&adapter->pend_list_lock' for the second time at 1892: > > // SECOND: DOUBLE LOCK !!! > spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags); > > From my shallow understanding of the code (so please review carefully), I think > that it is not necessary to hold `PENDING_LIST_LOCK(adapter)' while executing > the body of the `list_for_each_entry_safe' loop. I assume this because both > `megaraid_mbox_mm_done' and `megaraid_dealloc_scb' are called from > several places where, as far as I can tell, this lock is not hold. In fact, as reported > by EBA, at some point `megaraid_mbox_mm_done' will acquire this lock again. > > Fixes: c005fb4fb2d2 ("[SCSI] megaraid_{mm,mbox}: fix a bug in reset handler") > Signed-off-by: Iago Abal <mail@xxxxxxxxxxx> > --- > drivers/scsi/megaraid/megaraid_mbox.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/megaraid/megaraid_mbox.c > b/drivers/scsi/megaraid/megaraid_mbox.c > index f0987f2..7f11898 100644 > --- a/drivers/scsi/megaraid/megaraid_mbox.c > +++ b/drivers/scsi/megaraid/megaraid_mbox.c > @@ -2603,6 +2603,7 @@ static DEF_SCSI_QCMD(megaraid_queue_command) > list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) { > list_del_init(&scb->list); // from pending list > > + spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags); > if (scb->sno >= MBOX_MAX_SCSI_CMDS) { > con_log(CL_ANN, (KERN_WARNING > "megaraid: IOCTL packet with %d[%d:%d] being > reset\n", @@ -2630,6 +2631,7 @@ static > DEF_SCSI_QCMD(megaraid_queue_command) > > megaraid_dealloc_scb(adapter, scb); > } > + spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags); > } > spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags); Sorry for delay. We had internal discussion and confirm that it is safe to remove mbox driver from mainline as this product is discontinued and we are planning to post patch to remove megaraid mbox driver from mainline. > > -- > 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
