diff mbox series

scsi: mpt3sas: Fix unlock imbalance

Message ID 20200701085254.51740-1-damien.lemoal@wdc.com (mailing list archive)
State Mainlined
Commit cb551b8dc079d2ef189145782627c99cb68c0255
Headers show
Series scsi: mpt3sas: Fix unlock imbalance | expand

Commit Message

Damien Le Moal July 1, 2020, 8:52 a.m. UTC 
In BRM_status_show(), if the condition "!ioc->is_warpdrive" tested on
entry to the function is true, a "goto out" is called. This results in
unlocking ioc->pci_access_mutex without this mutex lock being taken.
This generates the following splat:

[ 1148.539883] mpt3sas_cm2: BRM_status_show: BRM attribute is only for warpdrive
[ 1148.547184]
[ 1148.548708] =====================================
[ 1148.553501] WARNING: bad unlock balance detected!
[ 1148.558277] 5.8.0-rc3+ #827 Not tainted
[ 1148.562183] -------------------------------------
[ 1148.566959] cat/5008 is trying to release lock (&ioc->pci_access_mutex) at:
[ 1148.574035] [<ffffffffc070b7a3>] BRM_status_show+0xd3/0x100 [mpt3sas]
[ 1148.580574] but there are no more locks to release!
[ 1148.585524]
[ 1148.585524] other info that might help us debug this:
[ 1148.599624] 3 locks held by cat/5008:
[ 1148.607085]  #0: ffff92aea3e392c0 (&p->lock){+.+.}-{3:3}, at: seq_read+0x34/0x480
[ 1148.618509]  #1: ffff922ef14c4888 (&of->mutex){+.+.}-{3:3}, at: kernfs_seq_start+0x2a/0xb0
[ 1148.630729]  #2: ffff92aedb5d7310 (kn->active#224){.+.+}-{0:0}, at: kernfs_seq_start+0x32/0xb0
[ 1148.643347]
[ 1148.643347] stack backtrace:
[ 1148.655259] CPU: 73 PID: 5008 Comm: cat Not tainted 5.8.0-rc3+ #827
[ 1148.665309] Hardware name: HGST H4060-S/S2600STB, BIOS SE5C620.86B.02.01.0008.031920191559 03/19/2019
[ 1148.678394] Call Trace:
[ 1148.684750]  dump_stack+0x78/0xa0
[ 1148.691802]  lock_release.cold+0x45/0x4a
[ 1148.699451]  __mutex_unlock_slowpath+0x35/0x270
[ 1148.707675]  BRM_status_show+0xd3/0x100 [mpt3sas]
[ 1148.716092]  dev_attr_show+0x19/0x40
[ 1148.723664]  sysfs_kf_seq_show+0x87/0x100
[ 1148.731193]  seq_read+0xbc/0x480
[ 1148.737882]  vfs_read+0xa0/0x160
[ 1148.744514]  ksys_read+0x58/0xd0
[ 1148.751129]  do_syscall_64+0x4c/0xa0
[ 1148.757941]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 1148.766240] RIP: 0033:0x7f1230566542
[ 1148.772957] Code: Bad RIP value.
[ 1148.779206] RSP: 002b:00007ffeac1bcac8 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
[ 1148.790063] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007f1230566542
[ 1148.800284] RDX: 0000000000020000 RSI: 00007f1223460000 RDI: 0000000000000003
[ 1148.810474] RBP: 00007f1223460000 R08: 00007f122345f010 R09: 0000000000000000
[ 1148.820641] R10: 0000000000000022 R11: 0000000000000246 R12: 0000000000000000
[ 1148.830728] R13: 0000000000000003 R14: 0000000000020000 R15: 0000000000020000

Fix this by returning immediately instead of jumping to the out label.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Johannes Thumshirn July 1, 2020, 10:10 a.m. UTC | #1 
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

While we're at it the next block does a direct return manually unlocking 
'ioc->pci_access_mutex' and rc is never set for any of the error paths 
in 'BRM_status_show'...

Maybe we should add this one on top of your patch:

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 62e552838565..70d2d0987249 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -3149,20 +3149,20 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
        }
        /* pci_access_mutex lock acquired by sysfs show path */
        mutex_lock(&ioc->pci_access_mutex);
-       if (ioc->pci_error_recovery || ioc->remove_host) {
-               mutex_unlock(&ioc->pci_access_mutex);
-               return 0;
-       }
+       if (ioc->pci_error_recovery || ioc->remove_host)
+               goto out;
 
        /* allocate upto GPIOVal 36 entries */
        sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
        io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
        if (!io_unit_pg3) {
+               rc = -ENOMEM;
                ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
                        __func__, sz);
                goto out;
        }
 
+       rc = -EINVAL;
        if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) !=
            0) {
                ioc_err(ioc, "%s: failed reading iounit_pg3\n",
Damien Le Moal July 1, 2020, 10:31 a.m. UTC | #2 
On 2020/07/01 19:10, Johannes Thumshirn wrote:
> Looks good,
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> While we're at it the next block does a direct return manually unlocking 
> 'ioc->pci_access_mutex' and rc is never set for any of the error paths 
> in 'BRM_status_show'...
> 
> Maybe we should add this one on top of your patch:
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 62e552838565..70d2d0987249 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -3149,20 +3149,20 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
>         }
>         /* pci_access_mutex lock acquired by sysfs show path */
>         mutex_lock(&ioc->pci_access_mutex);
> -       if (ioc->pci_error_recovery || ioc->remove_host) {
> -               mutex_unlock(&ioc->pci_access_mutex);
> -               return 0;
> -       }
> +       if (ioc->pci_error_recovery || ioc->remove_host)
> +               goto out;
>  
>         /* allocate upto GPIOVal 36 entries */
>         sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
>         io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
>         if (!io_unit_pg3) {
> +               rc = -ENOMEM;
>                 ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
>                         __func__, sz);
>                 goto out;
>         }
>  
> +       rc = -EINVAL;
>         if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) !=
>             0) {
>                 ioc_err(ioc, "%s: failed reading iounit_pg3\n",
> 

Indeed... I did not look at the other early return path :)
Can you send something ?
Johannes Thumshirn July 1, 2020, 12:46 p.m. UTC | #3 
On 01/07/2020 12:31, Damien Le Moal wrote:
[...]
> Indeed... I did not look at the other early return path :)
> Can you send something ?
> 


Sure
Sreekanth Reddy July 2, 2020, 7:45 a.m. UTC | #4 
Patch looks good.
Acked-by: sreekanth reddy <sreekanth.reddy@broadcom.com>

On Wed, Jul 1, 2020 at 3:40 PM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> Looks good,
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> While we're at it the next block does a direct return manually unlocking
> 'ioc->pci_access_mutex' and rc is never set for any of the error paths
> in 'BRM_status_show'...
>
> Maybe we should add this one on top of your patch:
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 62e552838565..70d2d0987249 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -3149,20 +3149,20 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr,
>         }
>         /* pci_access_mutex lock acquired by sysfs show path */
>         mutex_lock(&ioc->pci_access_mutex);
> -       if (ioc->pci_error_recovery || ioc->remove_host) {
> -               mutex_unlock(&ioc->pci_access_mutex);
> -               return 0;
> -       }
> +       if (ioc->pci_error_recovery || ioc->remove_host)
> +               goto out;
>
>         /* allocate upto GPIOVal 36 entries */
>         sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36);
>         io_unit_pg3 = kzalloc(sz, GFP_KERNEL);
>         if (!io_unit_pg3) {
> +               rc = -ENOMEM;
>                 ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n",
>                         __func__, sz);
>                 goto out;
>         }
>
> +       rc = -EINVAL;
>         if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) !=
>             0) {
>                 ioc_err(ioc, "%s: failed reading iounit_pg3\n",
Martin K. Petersen July 3, 2020, 4:02 a.m. UTC | #5 
On Wed, 1 Jul 2020 17:52:54 +0900, Damien Le Moal wrote:

> In BRM_status_show(), if the condition "!ioc->is_warpdrive" tested on
> entry to the function is true, a "goto out" is called. This results in
> unlocking ioc->pci_access_mutex without this mutex lock being taken.
> This generates the following splat:
> 
> [ 1148.539883] mpt3sas_cm2: BRM_status_show: BRM attribute is only for warpdrive
> [ 1148.547184]
> [ 1148.548708] =====================================
> [ 1148.553501] WARNING: bad unlock balance detected!
> [ 1148.558277] 5.8.0-rc3+ #827 Not tainted
> [ 1148.562183] -------------------------------------
> [ 1148.566959] cat/5008 is trying to release lock (&ioc->pci_access_mutex) at:
> [ 1148.574035] [<ffffffffc070b7a3>] BRM_status_show+0xd3/0x100 [mpt3sas]
> [ 1148.580574] but there are no more locks to release!
> [ 1148.585524]
> [ 1148.585524] other info that might help us debug this:
> [ 1148.599624] 3 locks held by cat/5008:
> [ 1148.607085]  #0: ffff92aea3e392c0 (&p->lock){+.+.}-{3:3}, at: seq_read+0x34/0x480
> [ 1148.618509]  #1: ffff922ef14c4888 (&of->mutex){+.+.}-{3:3}, at: kernfs_seq_start+0x2a/0xb0
> [ 1148.630729]  #2: ffff92aedb5d7310 (kn->active#224){.+.+}-{0:0}, at: kernfs_seq_start+0x32/0xb0
> [ 1148.643347]
> [ 1148.643347] stack backtrace:
> [ 1148.655259] CPU: 73 PID: 5008 Comm: cat Not tainted 5.8.0-rc3+ #827
> [ 1148.665309] Hardware name: HGST H4060-S/S2600STB, BIOS SE5C620.86B.02.01.0008.031920191559 03/19/2019
> [ 1148.678394] Call Trace:
> [ 1148.684750]  dump_stack+0x78/0xa0
> [ 1148.691802]  lock_release.cold+0x45/0x4a
> [ 1148.699451]  __mutex_unlock_slowpath+0x35/0x270
> [ 1148.707675]  BRM_status_show+0xd3/0x100 [mpt3sas]
> [ 1148.716092]  dev_attr_show+0x19/0x40
> [ 1148.723664]  sysfs_kf_seq_show+0x87/0x100
> [ 1148.731193]  seq_read+0xbc/0x480
> [ 1148.737882]  vfs_read+0xa0/0x160
> [ 1148.744514]  ksys_read+0x58/0xd0
> [ 1148.751129]  do_syscall_64+0x4c/0xa0
> [ 1148.757941]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1148.766240] RIP: 0033:0x7f1230566542
> [ 1148.772957] Code: Bad RIP value.
> [ 1148.779206] RSP: 002b:00007ffeac1bcac8 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> [ 1148.790063] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007f1230566542
> [ 1148.800284] RDX: 0000000000020000 RSI: 00007f1223460000 RDI: 0000000000000003
> [ 1148.810474] RBP: 00007f1223460000 R08: 00007f122345f010 R09: 0000000000000000
> [ 1148.820641] R10: 0000000000000022 R11: 0000000000000246 R12: 0000000000000000
> [ 1148.830728] R13: 0000000000000003 R14: 0000000000020000 R15: 0000000000020000
> 
> [...]

Applied to 5.8/scsi-fixes, thanks!

[1/1] scsi: mpt3sas: Fix unlock imbalance
      https://git.kernel.org/mkp/scsi/c/cb551b8dc079
diff mbox series

Patch

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index 62e552838565..e94e72de2fc6 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -3145,7 +3145,7 @@  BRM_status_show(struct device *cdev, struct device_attribute *attr,
 	if (!ioc->is_warpdrive) {
 		ioc_err(ioc, "%s: BRM attribute is only for warpdrive\n",
 			__func__);
-		goto out;
+		return 0;
 	}
 	/* pci_access_mutex lock acquired by sysfs show path */
 	mutex_lock(&ioc->pci_access_mutex);