diff mbox

[v1,03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.

Message ID 1522943197-5408-4-git-send-email-chaitra.basappa@broadcom.com (mailing list archive)
State Superseded
Headers show

Commit Message

Chaitra P B April 5, 2018, 3:46 p.m. UTC 
Check scsi tracker 'st' for NULL and st->smid for zero (as driver uses smid
starting from one) before accessing it.
These checks are added as there are possibilities for getting valid
scsi_cmd when driver calls scsi_host_find_tag() API when it loops using
smid(i.e tag) from one to hba queue depth but still scsi tracker st for
this corresponding scsi_cmd is not yet initialized.

For example below are such scenario:
Sometimes it is possible that scsi_cmd might have created at SML but it
might not be issued to the driver (or driver might have returned the
command with Host busy status) as the host reset operation / TMs is in
progress.In such case where the scsi_cmd is not yet processed by driver
then the scsi tracker 'st' of that scsi_cmd & the fields of this 'st' will
be uninitialized.
And hence this patch add checks for 'st' in IOCTL path for TMs issued from
applications and also in host reset path where driver flushes all the
outstanding commands as part of host reset operation.

Signed-off-by: Chaitra P B <chaitra.basappa@broadcom.com>
Signed-off-by: Suganath Prabu S <suganath-prabu.subramani@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c   | 5 ++++-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 9 ++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Bart Van Assche April 6, 2018, 3:28 p.m. UTC | #1 
On Thu, 2018-04-05 at 11:46 -0400, Chaitra P B wrote:
> Check scsi tracker 'st' for NULL and st->smid for zero (as driver uses smid

> starting from one) before accessing it.

> These checks are added as there are possibilities for getting valid

> scsi_cmd when driver calls scsi_host_find_tag() API when it loops using

> smid(i.e tag) from one to hba queue depth but still scsi tracker st for

> this corresponding scsi_cmd is not yet initialized.

> 

> For example below are such scenario:

> Sometimes it is possible that scsi_cmd might have created at SML but it

> might not be issued to the driver (or driver might have returned the

> command with Host busy status) as the host reset operation / TMs is in

> progress.In such case where the scsi_cmd is not yet processed by driver

> then the scsi tracker 'st' of that scsi_cmd & the fields of this 'st' will

> be uninitialized.

> And hence this patch add checks for 'st' in IOCTL path for TMs issued from

> applications and also in host reset path where driver flushes all the

> outstanding commands as part of host reset operation.


What is needed is an explanation about which mechanism serializes the
execution of scsih_qcmd() and mpt3sas_base_hard_reset_handler(), at least if
such a mechanism exists. The above text does not mention anything about such
a synchronization mechanism.
 
>  		scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);

> -		if (!scmd)

> +		if (scmd == NULL || scmd->device == NULL ||

> +				scmd->device->hostdata == NULL)


As Christoph explained before, scmd->device is never NULL. Additionally, the
scmd->device->hostdata check looks very suspicious. That check should either
be left out or the race that causes a SCSI device to be removed concurrently
with this function should be fixed.

If you are unable to motivate why this patch is correct, please repost this
series without this patch.

Thanks,

Bart.
Chaitra P B April 9, 2018, 8:18 a.m. UTC | #2 
Bart,
 We will work on this patch and submit. As of now reposting all the patches
of this series except this patch.

Thanks,
 Chaitra


-----Original Message-----
From: Bart Van Assche [mailto:Bart.VanAssche@wdc.com]
Sent: Friday, April 6, 2018 8:59 PM
To: chaitra.basappa@broadcom.com; linux-scsi@vger.kernel.org
Cc: Sathya.Prakash@broadcom.com; sreekanth.reddy@broadcom.com;
suganath-prabu.subramani@broadcom.com
Subject: Re: [PATCH v1 03/15] mpt3sas: Add sanity checks for scsi tracker
before accessing it.

On Thu, 2018-04-05 at 11:46 -0400, Chaitra P B wrote:
> Check scsi tracker 'st' for NULL and st->smid for zero (as driver uses
> smid starting from one) before accessing it.
> These checks are added as there are possibilities for getting valid
> scsi_cmd when driver calls scsi_host_find_tag() API when it loops
> using smid(i.e tag) from one to hba queue depth but still scsi tracker
> st for this corresponding scsi_cmd is not yet initialized.
>
> For example below are such scenario:
> Sometimes it is possible that scsi_cmd might have created at SML but
> it might not be issued to the driver (or driver might have returned
> the command with Host busy status) as the host reset operation / TMs
> is in progress.In such case where the scsi_cmd is not yet processed by
> driver then the scsi tracker 'st' of that scsi_cmd & the fields of
> this 'st' will be uninitialized.
> And hence this patch add checks for 'st' in IOCTL path for TMs issued
> from applications and also in host reset path where driver flushes all
> the outstanding commands as part of host reset operation.

What is needed is an explanation about which mechanism serializes the
execution of scsih_qcmd() and mpt3sas_base_hard_reset_handler(), at least if
such a mechanism exists. The above text does not mention anything about such
a synchronization mechanism.

>  		scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
> -		if (!scmd)
> +		if (scmd == NULL || scmd->device == NULL ||
> +				scmd->device->hostdata == NULL)

As Christoph explained before, scmd->device is never NULL. Additionally, the
scmd->device->hostdata check looks very suspicious. That check should
scmd->device->either
be left out or the race that causes a SCSI device to be removed concurrently
with this function should be fixed.

If you are unable to motivate why this patch is correct, please repost this
series without this patch.

Thanks,

Bart.
Sasha Levin April 10, 2018, 1:49 p.m. UTC | #3 
Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined
to be a high probability candidate for -stable trees. (score: 28.7865)

The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33, v4.9.93, v4.4.127.

v4.16.1: Build OK!
v4.15.16: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.14.33: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.9.93: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.4.127: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")


Please let us know if you'd like to have this patch included in a stable tree.

--
Thanks,
Sasha
Sreekanth Reddy April 10, 2018, 4:49 p.m. UTC | #4 
Yes Sasha, We like to have this patch included in a stable tree.

Thanks,
Sreekanth

-----Original Message-----
From: Sasha Levin [mailto:Alexander.Levin@microsoft.com]
Sent: Tuesday, April 10, 2018 7:19 PM
To: Sasha Levin; Chaitra P B; linux-scsi@vger.kernel.org
Cc: Sathya.Prakash@broadcom.com; sreekanth.reddy@broadcom.com;
stable@vger.kernel.org
Subject: Re: [PATCH v1 03/15] mpt3sas: Add sanity checks for scsi tracker
before accessing it.

Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined to
be a high probability candidate for -stable trees. (score: 28.7865)

The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33,
v4.9.93, v4.4.127.

v4.16.1: Build OK!
v4.15.16: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.14.33: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.9.93: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")

v4.4.127: Failed to apply! Possible dependencies:
    dbec4c9040ed ("scsi: mpt3sas: lockless command submission")


Please let us know if you'd like to have this patch included in a stable
tree.

--
Thanks,
Sasha
diff mbox

Patch

diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index c1b17d6..2f27d5c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -590,7 +590,8 @@  _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,
 		struct scsiio_tracker *st;
 
 		scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
-		if (!scmd)
+		if (scmd == NULL || scmd->device == NULL ||
+				scmd->device->hostdata == NULL)
 			continue;
 		if (lun != scmd->device->lun)
 			continue;
@@ -600,6 +601,8 @@  _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,
 		if (priv_data->sas_target->handle != handle)
 			continue;
 		st = scsi_cmd_priv(scmd);
+		if ((!st) || (st->smid == 0))
+			continue;
 		tm_request->TaskMID = cpu_to_le16(st->smid);
 		found = 1;
 	}
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index c9cce65..6b1aaa0 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -1465,7 +1465,7 @@  mpt3sas_scsih_scsi_lookup_get(struct MPT3SAS_ADAPTER *ioc, u16 smid)
 		scmd = scsi_host_find_tag(ioc->shost, unique_tag);
 		if (scmd) {
 			st = scsi_cmd_priv(scmd);
-			if (st->cb_idx == 0xFF)
+			if ((!st) || (st->cb_idx == 0xFF) || (st->smid == 0))
 				scmd = NULL;
 		}
 	}
@@ -4451,6 +4451,13 @@  _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
 		count++;
 		_scsih_set_satl_pending(scmd, false);
 		st = scsi_cmd_priv(scmd);
+		/*
+		 * It may be possible that SCSI scmd got prepared by SML
+		 * but it has not issued to the driver, for these type of
+		 * scmd's don't do anything"
+		 */
+		if (st && st->smid == 0)
+			continue;
 		mpt3sas_base_clear_st(ioc, st);
 		scsi_dma_unmap(scmd);
 		if (ioc->pci_error_recovery)