| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170423083635.GB30869@infradead.org>
Date: Sun, 23 Apr 2017 01:36:35 -0700
From: Christoph Hellwig <hch@...radead.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
stable@...r.kernel.org, Johannes Berg <johannes.berg@...el.com>,
Quentin Lambert <lambert.quentin@...il.com>,
linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] scsi: pmcraid: fix lock imbalance in
pmcraid_reset_reload()
On Thu, Apr 20, 2017 at 07:54:46PM +0200, Arnd Bergmann wrote:
> sparse found a bug that has always been present since the driver
> was merged:
>
> drivers/scsi/pmcraid.c:2353:12: warning: context imbalance in 'pmcraid_reset_reload' - different lock contexts for basic block
>
> This adds the missing unlock at the end of the function. I could
> not figure out if this will happen in practice, but I could not
> prove that it doesn't happen, so to be on the safe side, let's
> backport this fix.
>
> Cc: stable@...r.kernel.org
> Fixes: 89a368104150 ("[SCSI] pmcraid: PMC-Sierra MaxRAID driver to support 6Gb/s SAS RAID controller")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> drivers/scsi/pmcraid.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index 096c704ca39a..35087e94c2ad 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -2411,8 +2411,11 @@ static int pmcraid_reset_reload(
> scsi_unblock_requests(pinstance->host);
> if (pinstance->ioa_state == target_state)
> reset = 0;
> + spin_lock_irqsave(pinstance->host->host_lock, lock_flags);
> }
>
> + spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
> +
This looks weird. Using a proper goto label to unwind seem like the
best way to go, e.g. this patch:
---
>From 8c58854f345bd87789b68eba2b2f72d0cac952fa Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@....de>
Date: Sun, 23 Apr 2017 10:33:23 +0200
Subject: pmcraid: fix lock imbalance in pmcraid_reset_reload()
sparse found a bug that has always been present since the driver was
merged:
drivers/scsi/pmcraid.c:2353:12: warning: context imbalance in 'pmcraid_reset_reload' - different lock contexts for basic block
Fix this by using a common unlock goto label, and also reduce the
indentation level in the function.
Signed-off-by: Christoph Hellwig <hch@....de>
Reported-by: Arnd Bergmann <arnd@...db.de>
---
drivers/scsi/pmcraid.c | 59 ++++++++++++++++++++++++--------------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 49e70a383afa..3cc858f45838 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -2373,46 +2373,43 @@ static int pmcraid_reset_reload(
spin_lock_irqsave(pinstance->host->host_lock, lock_flags);
if (pinstance->ioa_state == IOA_STATE_DEAD) {
- spin_unlock_irqrestore(pinstance->host->host_lock,
- lock_flags);
pmcraid_info("reset_reload: IOA is dead\n");
- return reset;
- } else if (pinstance->ioa_state == target_state) {
+ goto out_unlock;
+ }
+
+ if (pinstance->ioa_state == target_state) {
reset = 0;
+ goto out_unlock;
}
}
- if (reset) {
- pmcraid_info("reset_reload: proceeding with reset\n");
- scsi_block_requests(pinstance->host);
- reset_cmd = pmcraid_get_free_cmd(pinstance);
-
- if (reset_cmd == NULL) {
- pmcraid_err("no free cmnd for reset_reload\n");
- spin_unlock_irqrestore(pinstance->host->host_lock,
- lock_flags);
- return reset;
- }
+ pmcraid_info("reset_reload: proceeding with reset\n");
+ scsi_block_requests(pinstance->host);
+ reset_cmd = pmcraid_get_free_cmd(pinstance);
+ if (reset_cmd == NULL) {
+ pmcraid_err("no free cmnd for reset_reload\n");
+ goto out_unlock;
+ }
- if (shutdown_type == SHUTDOWN_NORMAL)
- pinstance->ioa_bringdown = 1;
+ if (shutdown_type == SHUTDOWN_NORMAL)
+ pinstance->ioa_bringdown = 1;
- pinstance->ioa_shutdown_type = shutdown_type;
- pinstance->reset_cmd = reset_cmd;
- pinstance->force_ioa_reset = reset;
- pmcraid_info("reset_reload: initiating reset\n");
- pmcraid_ioa_reset(reset_cmd);
- spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
- pmcraid_info("reset_reload: waiting for reset to complete\n");
- wait_event(pinstance->reset_wait_q,
- !pinstance->ioa_reset_in_progress);
+ pinstance->ioa_shutdown_type = shutdown_type;
+ pinstance->reset_cmd = reset_cmd;
+ pinstance->force_ioa_reset = reset;
+ pmcraid_info("reset_reload: initiating reset\n");
+ pmcraid_ioa_reset(reset_cmd);
+ spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
+ pmcraid_info("reset_reload: waiting for reset to complete\n");
+ wait_event(pinstance->reset_wait_q,
+ !pinstance->ioa_reset_in_progress);
- pmcraid_info("reset_reload: reset is complete !!\n");
- scsi_unblock_requests(pinstance->host);
- if (pinstance->ioa_state == target_state)
- reset = 0;
- }
+ pmcraid_info("reset_reload: reset is complete !!\n");
+ scsi_unblock_requests(pinstance->host);
+ return pinstance->ioa_state != target_state;
+out_unlock:
+ spin_unlock_irqrestore(pinstance->host->host_lock, lock_flags);
return reset;
}
--
2.11.0
Powered by blists - more mailing lists