RE: [PATCH v1 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.
Yes Sasha, We like to have this patch included in a stable tree.
Thanks,
Sreekanth
-Original Message-
From: Sasha Levin [mailto:alexander.le...@microsoft.com]
Sent: Tuesday, April 10, 2018 7:19 PM
To: Sasha Levin; Chaitra P B; linux-scsi@vger.kernel.org
Cc: sathya.prak...@broadcom.com; sreekanth.re...@broadcom.com;
sta...@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
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
RE: [PATCH v1 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.
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.vanass...@wdc.com] Sent: Friday, April 6, 2018 8:59 PM To: chaitra.basa...@broadcom.com; linux-scsi@vger.kernel.org Cc: sathya.prak...@broadcom.com; sreekanth.re...@broadcom.com; suganath-prabu.subram...@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.
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 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.
