* [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock
@ 2016-10-17 7:30 iari
2016-10-20 8:34 ` Kashyap Desai
2016-11-11 4:48 ` Kashyap Desai
0 siblings, 2 replies; 3+ messages in thread
From: iari @ 2016-10-17 7:30 UTC (permalink / raw)
To: Jiri Kosina
Cc: Kashyap Desai, Sumit Saxena, Uday Lingala, James E.J. Bottomley,
Martin K. Petersen, megaraidlinux.pdl, linux-scsi, Iago Abal
From: Iago Abal <mail@iagoabal.eu>
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@iagoabal.eu>
---
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);
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock
2016-10-17 7:30 [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock iari
@ 2016-10-20 8:34 ` Kashyap Desai
2016-11-11 4:48 ` Kashyap Desai
1 sibling, 0 replies; 3+ messages in thread
From: Kashyap Desai @ 2016-10-20 8:34 UTC (permalink / raw)
To: iari, Jiri Kosina
Cc: Kashyap Desai, Sumit Saxena, Uday Lingala, James E.J. Bottomley,
Martin K. Petersen, megaraidlinux.pdl, linux-scsi, Iago Abal
> -----Original Message-----
> From: iari@itu.dk [mailto:iari@itu.dk]
> 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@avagotech.com; linux-scsi@vger.kernel.org;
Iago
> Abal
> Subject: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list
lock to
> avoid double lock
>
> From: Iago Abal <mail@iagoabal.eu>
>
> 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@iagoabal.eu>
> ---
> 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);
Looks correct, but please note that MEGARAID_MAILBOX and MEGARAID_MM is
not supported by LSI/ Broadcom. We will revert back to you shortly if we
can safely remove those two modules.
.
>
> --
> 1.9.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock
2016-10-17 7:30 [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock iari
2016-10-20 8:34 ` Kashyap Desai
@ 2016-11-11 4:48 ` Kashyap Desai
1 sibling, 0 replies; 3+ messages in thread
From: Kashyap Desai @ 2016-11-11 4:48 UTC (permalink / raw)
To: iari, Jiri Kosina
Cc: Kashyap Desai, Sumit Saxena, Uday Lingala, James E.J. Bottomley,
Martin K. Petersen, megaraidlinux.pdl, linux-scsi, Iago Abal
> -----Original Message-----
> From: iari@itu.dk [mailto:iari@itu.dk]
> 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@avagotech.com; linux-scsi@vger.kernel.org;
Iago
> Abal
> Subject: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list
lock to
> avoid double lock
>
> From: Iago Abal <mail@iagoabal.eu>
>
> 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@iagoabal.eu>
> ---
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-11 4:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-17 7:30 [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock iari
2016-10-20 8:34 ` Kashyap Desai
2016-11-11 4:48 ` Kashyap Desai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).