[PATCH v2] scsi: mpt3sas: fix oops in error handlers after shutdown/unload

* [PATCH v2] scsi: mpt3sas: fix oops in error handlers after shutdown/unload
@ 2018-02-01 22:16 Mauricio Faria de Oliveira
  2018-02-01 22:27 ` test results Mauricio Faria de Oliveira
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Mauricio Faria de Oliveira @ 2018-02-01 22:16 UTC (permalink / raw)
  To: linux-scsi, Bart.VanAssche
  Cc: sathya.prakash, chaitra.basappa, suganath-prabu.subramani, jejb,
	martin.petersen, dougmill

This patch adds checks for 'ioc->remove_host' in the SCSI error
handlers, so not to access pointers/resources potentially freed
in the PCI shutdown/module unload path.  The error handlers may
be invoked after shutdown/unload, depending on other components.

This problem was observed with kexec on a system with a mpt3sas
based adapter and an infiniband adapter which takes long enough
to shutdown:

The mpt3sas driver finished shutting down / disabled interrupt
handling, thus some commands have not finished and timed out.

Since the system was still running (waiting for the infiniband
adapter to shutdown), the scsi error handler for task abort of
mpt3sas was invoked, and hit an oops -- either in scsih_abort()
because 'ioc->scsi_lookup' was NULL (without commit dbec4c9040ed
("scsi: mpt3sas: lockless command submission")), or later up in
scsih_host_reset() (with or without that commit), because it
eventually called mpt3sas_base_get_iocstate().

After that commit, the oops in scsih_abort() does not occur
anymore (_scsih_scsi_lookup_find_by_scmd() is no longer called),
but that commit is too big and out of the scope of linux-stable,
where this patch might help, so still go for the changes.

Also, this might help to prevent similar errors in the future,
in case code changes and possibly tries to access freed stuff.

Note the fix in scsih_host_reset() is still important anyway.

Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
---
v2:
 - rebase on top of mkp/scsi.git's fixes branch
 - insert check for 'ioc->remove_host' in existing
   checks which already set DID_NO_CONNECT instead
   of duplicating that code. (helps with backports)
 - update commit message about that commit.

 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 74fca18..5ab3caf 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2835,7 +2835,8 @@ int mpt3sas_scsih_issue_locked_tm(struct MPT3SAS_ADAPTER *ioc, u16 handle,
 	_scsih_tm_display_info(ioc, scmd);
 
 	sas_device_priv_data = scmd->device->hostdata;
-	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
+	if (!sas_device_priv_data || !sas_device_priv_data->sas_target ||
+	    ioc->remove_host) {
 		sdev_printk(KERN_INFO, scmd->device,
 			"device been deleted! scmd(%p)\n", scmd);
 		scmd->result = DID_NO_CONNECT << 16;
@@ -2898,7 +2899,8 @@ int mpt3sas_scsih_issue_locked_tm(struct MPT3SAS_ADAPTER *ioc, u16 handle,
 	_scsih_tm_display_info(ioc, scmd);
 
 	sas_device_priv_data = scmd->device->hostdata;
-	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
+	if (!sas_device_priv_data || !sas_device_priv_data->sas_target ||
+	    ioc->remove_host) {
 		sdev_printk(KERN_INFO, scmd->device,
 			"device been deleted! scmd(%p)\n", scmd);
 		scmd->result = DID_NO_CONNECT << 16;
@@ -2961,7 +2963,8 @@ int mpt3sas_scsih_issue_locked_tm(struct MPT3SAS_ADAPTER *ioc, u16 handle,
 	_scsih_tm_display_info(ioc, scmd);
 
 	sas_device_priv_data = scmd->device->hostdata;
-	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
+	if (!sas_device_priv_data || !sas_device_priv_data->sas_target ||
+	    ioc->remove_host) {
 		starget_printk(KERN_INFO, starget, "target been deleted! scmd(%p)\n",
 			scmd);
 		scmd->result = DID_NO_CONNECT << 16;
@@ -3019,7 +3022,7 @@ int mpt3sas_scsih_issue_locked_tm(struct MPT3SAS_ADAPTER *ioc, u16 handle,
 	    ioc->name, scmd);
 	scsi_print_command(scmd);
 
-	if (ioc->is_driver_loading) {
+	if (ioc->is_driver_loading || ioc->remove_host) {
 		pr_info(MPT3SAS_FMT "Blocking the host reset\n",
 		    ioc->name);
 		r = FAILED;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread