RE: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock

[Kashyap Desai <kashyap.desai@broadcom.com>]

> -----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