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

Commit Message

Sreekanth Reddy Feb. 12, 2018, 6:28 a.m. UTC

Mauricio,

Instead of returning the scmd with DID_NO_CONNECT in TM path, can we wait
for some time (10 seconds) in shutdown/unload path for the outstanding
commands to complete and even then the scmds are outstanding then return
all the outstanding scmds with DID_NO_CONNECT in the shutdown/unload path
itself as shown below,

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

Comments

Mauricio Faria de Oliveira Feb. 14, 2018, 3:35 p.m. UTC | #1

Hi Sreenkanth,

Thanks for reviewing.

On 02/12/2018 04:28 AM, Sreekanth Reddy wrote:
> Instead of returning the scmd with DID_NO_CONNECT in TM path, can we wait
> for some time (10 seconds) in shutdown/unload path for the outstanding
> commands to complete and even then [if] the scmds are outstanding then return
> all the outstanding scmds with DID_NO_CONNECT in the shutdown/unload path
> itself as shown below,

Apparently there is a problem with that.

That is not enough for TM commands; it's only for SCSI IO commands.

The TM commands use the high-priority queue, and SCSI IO commands use
the SCSI IO queue.  But this patch only waits for and return commands
in the latter:

 > +	mpt3sas_wait_for_commands_to_complete(ioc);
 > +	_scsih_flush_running_cmds(ioc);

They only loop up until 'ioc->scsiio_depth', but TM commands get SMIDs
above that [see mpt3sas_base_free_smid() and mpt3sas_scsih_issue_tm()].

So, there's an exposure for abort/reset/other TM commands. For example,
the SCSI IO commands finish quickly in wait_for_commands_to_complete(),
then the shutdown function continues, disables interrupts, and release
pointers/memory.  Then, an outstanding abort/reset command is finished,
its completion interrupt is not serviced, and the SCSI EH for it kicks
in/continues, and so it escalates up, and hit that oops in host reset.

Is there any problem with the patch that I proposed?

It seems okay to have another check in error path (not hot/performance
path), and that check is already used in many other points in the code,
for the same reasons (exit early before the code attempts to use stuff
that might be released).

Thanks again,

Sreekanth Reddy Feb. 15, 2018, 5:48 a.m. UTC | #2

Mauricio,

During the shutdown time, I don't want the outstanding IOs to timeout due to
disabling of interrupts and go the TM path. So I wanted to clear out all the
Outstanding IOs in the shutdown path itself instead of clearing them in TM
path.

But if there are already some TMs are outstanding while initiating the
shutdown operation then your patch looks good to me.

May be can you include my set of changes along with your solution?

Thanks,
Sreekanth

-----Original Message-----
From: Mauricio Faria de Oliveira [mailto:[e-mail]]
Sent: Wednesday, February 14, 2018 9:05 PM
To: Sreekanth Reddy; [e-mail]; [e-mail]
Cc: Sathya Prakash Veerichetty; Chaitra Basappa; Suganath Prabu Subramani;
[e-mail]; [e-mail];
[e-mail]
Subject: Re: [PATCH v2] scsi: mpt3sas: fix oops in error handlers after
shutdown/unload

Hi Sreenkanth,

Thanks for reviewing.

On 02/12/2018 04:28 AM, Sreekanth Reddy wrote:
> Instead of returning the scmd with DID_NO_CONNECT in TM path, can we
> wait for some time (10 seconds) in shutdown/unload path for the
> outstanding commands to complete and even then [if] the scmds are
> outstanding then return all the outstanding scmds with DID_NO_CONNECT
> in the shutdown/unload path itself as shown below,

Apparently there is a problem with that.

That is not enough for TM commands; it's only for SCSI IO commands.

The TM commands use the high-priority queue, and SCSI IO commands use the
SCSI IO queue.  But this patch only waits for and return commands in the
latter:

 > +	mpt3sas_wait_for_commands_to_complete(ioc);
 > +	_scsih_flush_running_cmds(ioc);

They only loop up until 'ioc->scsiio_depth', but TM commands get SMIDs above
that [see mpt3sas_base_free_smid() and mpt3sas_scsih_issue_tm()].

So, there's an exposure for abort/reset/other TM commands. For example, the
SCSI IO commands finish quickly in wait_for_commands_to_complete(), then the
shutdown function continues, disables interrupts, and release
pointers/memory.  Then, an outstanding abort/reset command is finished, its
completion interrupt is not serviced, and the SCSI EH for it kicks
in/continues, and so it escalates up, and hit that oops in host reset.

Is there any problem with the patch that I proposed?

It seems okay to have another check in error path (not hot/performance
path), and that check is already used in many other points in the code, for
the same reasons (exit early before the code attempts to use stuff that
might be released).

Thanks again,

--
Mauricio Faria de Oliveira
IBM Linux Technology Center

Mauricio Faria de Oliveira Feb. 15, 2018, 12:10 p.m. UTC | #3

Hi Sreekanth,

On 02/15/2018 03:48 AM, Sreekanth Reddy wrote:
> During the shutdown time, I don't want the outstanding IOs to timeout due to
> disabling of interrupts and go the TM path. So I wanted to clear out all the
> Outstanding IOs in the shutdown path itself instead of clearing them in TM
> path.

Ah. I see. I didn't realize that. Good catch.

> But if there are already some TMs are outstanding while initiating the
> shutdown operation then your patch looks good to me.

Ok. Thanks for the review.

> May be can you include my set of changes along with your solution?

Sure. I'll send a v3 with both parts and due credit.

cheers,
Mauricio

Patch

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 13d6e4e..f62ce50 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6294,14 +6294,14 @@  _base_reset_handler(struct MPT3SAS_ADAPTER *ioc,
int reset_phase)
 }

 /**
- * _wait_for_commands_to_complete - reset controller
+ * mpt3sas_wait_for_commands_to_complete - reset controller
  * @ioc: Pointer to MPT_ADAPTER structure
  *
  * This function is waiting 10s for all pending commands to complete
  * prior to putting controller in reset.
  */
-static void
-_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc)
+void
+mpt3sas_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc)
 {
 	u32 ioc_state;

@@ -6374,7 +6374,7 @@  mpt3sas_base_hard_reset_handler(struct
MPT3SAS_ADAPTER *ioc,
 			is_fault = 1;
 	}
 	_base_reset_handler(ioc, MPT3_IOC_PRE_RESET);
-	_wait_for_commands_to_complete(ioc);
+	mpt3sas_wait_for_commands_to_complete(ioc);
 	_base_mask_interrupts(ioc);
 	r = _base_make_ioc_ready(ioc, type);
 	if (r)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 789bc42..99ccf83 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -1433,6 +1433,9 @@  void mpt3sas_base_update_missing_delay(struct
MPT3SAS_ADAPTER *ioc,

 int mpt3sas_port_enable(struct MPT3SAS_ADAPTER *ioc);

+void
+mpt3sas_wait_for_commands_to_complete(struct MPT3SAS_ADAPTER *ioc);
+

 /* scsih shared API */
 struct scsi_cmnd *mpt3sas_scsih_scsi_lookup_get(struct MPT3SAS_ADAPTER
*ioc,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 74fca18..458709e 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4453,7 +4453,7 @@  _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER
*ioc)
 		st = scsi_cmd_priv(scmd);
 		mpt3sas_base_clear_st(ioc, st);
 		scsi_dma_unmap(scmd);
-		if (ioc->pci_error_recovery)
+		if (ioc->pci_error_recovery || ioc->remove_host)
 			scmd->result = DID_NO_CONNECT << 16;
 		else
 			scmd->result = DID_RESET << 16;
@@ -9739,6 +9739,10 @@  static void scsih_remove(struct pci_dev *pdev)
 	unsigned long flags;

 	ioc->remove_host = 1;
+
+	mpt3sas_wait_for_commands_to_complete(ioc);
+	_scsih_flush_running_cmds(ioc);
+
 	_scsih_fw_event_cleanup_queue(ioc);

 	spin_lock_irqsave(&ioc->fw_event_lock, flags);
@@ -9815,6 +9819,10 @@  scsih_shutdown(struct pci_dev *pdev)
 	unsigned long flags;

 	ioc->remove_host = 1;
+
+	mpt3sas_wait_for_commands_to_complete(ioc);
+	_scsih_flush_running_cmds(ioc);
+
 	_scsih_fw_event_cleanup_queue(ioc);

 	spin_lock_irqsave(&ioc->fw_event_lock, flags);
---

-----Original Message-----
From: [e-mail]
[mailto:[e-mail]] On Behalf Of Mauricio Faria de
Oliveira
Sent: Friday, February 2, 2018 3:46 AM
To: [e-mail]; [e-mail]
Cc: [e-mail]; [e-mail];
[e-mail]; [e-mail];
[e-mail]; [e-mail]
Subject: [PATCH v2] scsi: mpt3sas: fix oops in error handlers after
shutdown/unload

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 <[e-mail]>
---
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);