[PATCH V2 0/2] scsi: ufs: core: Fix task management completion timeout race

[PATCH V2 0/2] scsi: ufs: core: Fix task management completion timeout race

[Adrian Hunter]

Hi

I though I sent this back on 15 October, but apparently I didn't, so
here it is now.

Please consider this for fixes.


Changes in V2:

      scsi: ufs: core: Fix task management completion timeout race
	Add Bart's Reviewed-by

      scsi: ufs: core: Fix another task management completion race
	New patch


Adrian Hunter (2):
      scsi: ufs: core: Fix task management completion timeout race
      scsi: ufs: core: Fix another task management completion race

 drivers/scsi/ufs/ufshcd.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)


Regards
Adrian

[PATCH V2 1/2] scsi: ufs: core: Fix task management completion timeout race

[Adrian Hunter]

__ufshcd_issue_tm_cmd() clears req->end_io_data after timing out,
which races with the completion function ufshcd_tmc_handler() which
expects req->end_io_data to have a value.

Note __ufshcd_issue_tm_cmd() and ufshcd_tmc_handler() are already
synchronized using hba->tmf_rqs and hba->outstanding_tasks under the
host_lock spinlock.

It is also not necessary (nor typical) to clear req->end_io_data because
the block layer does it before allocating out requests e.g. via
blk_get_request().

So fix by not clearing it.

Fixes: f5ef336fd2e4c3 ("scsi: ufs: core: Fix task management completion")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 470affdec426..a904531ba528 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6616,11 +6616,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	err = wait_for_completion_io_timeout(&wait,
 			msecs_to_jiffies(TM_CMD_TIMEOUT));
 	if (!err) {
-		/*
-		 * Make sure that ufshcd_compl_tm() does not trigger a
-		 * use-after-free.
-		 */
-		req->end_io_data = NULL;
 		ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_ERR);
 		dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
 				__func__, tm_function);
-- 
2.25.1

[PATCH V2 2/2] scsi: ufs: core: Fix another task management completion race

[Adrian Hunter]

hba->outstanding_tasks, which is read under host_lock spinlock, tells
the interrupt handler what task management tags are in use by the driver.
The doorbell register bits indicate which tags are in use by the hardware.
A doorbell bit that is 0 is because the bit has yet to be set by the
driver, or because the task is complete. It is only possible to
disambiguate the 2 cases, if reading/writing the doorbell register is
synchronized with reading/writing hba->outstanding_tasks.

For that reason, reading REG_UTP_TASK_REQ_DOOR_BELL must be done under
spinlock.

Fixes: f5ef336fd2e4c3 ("scsi: ufs: core: Fix task management completion")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a904531ba528..a1519b2b6cfe 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6453,9 +6453,8 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
 	irqreturn_t ret = IRQ_NONE;
 	int tag;
 
-	pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
-
 	spin_lock_irqsave(hba->host->host_lock, flags);
+	pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
 	issued = hba->outstanding_tasks & ~pending;
 	for_each_set_bit(tag, &issued, hba->nutmrs) {
 		struct request *req = hba->tmf_rqs[tag];
-- 
2.25.1

Re: [PATCH V2 2/2] scsi: ufs: core: Fix another task management completion race

[Bart Van Assche]

On 11/7/21 10:48 PM, Adrian Hunter wrote:
> hba->outstanding_tasks, which is read under host_lock spinlock, tells
> the interrupt handler what task management tags are in use by the driver.
> The doorbell register bits indicate which tags are in use by the hardware.
> A doorbell bit that is 0 is because the bit has yet to be set by the
> driver, or because the task is complete. It is only possible to
> disambiguate the 2 cases, if reading/writing the doorbell register is
> synchronized with reading/writing hba->outstanding_tasks.
> 
> For that reason, reading REG_UTP_TASK_REQ_DOOR_BELL must be done under
> spinlock.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH V2 0/2] scsi: ufs: core: Fix task management completion timeout race

[Martin K. Petersen]

Adrian,

> I though I sent this back on 15 October, but apparently I didn't, so
> here it is now.
>
> Please consider this for fixes.

Applied to 5.16/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2 0/2] scsi: ufs: core: Fix task management completion timeout race

[Martin K. Petersen]

On Mon, 8 Nov 2021 08:48:13 +0200, Adrian Hunter wrote:

> I though I sent this back on 15 October, but apparently I didn't, so
> here it is now.
> 
> Please consider this for fixes.
> 
> 
> Changes in V2:
> 
> [...]

Applied to 5.16/scsi-fixes, thanks!

[1/2] scsi: ufs: core: Fix task management completion timeout race
      https://git.kernel.org/mkp/scsi/c/886fe2915cce
[2/2] scsi: ufs: core: Fix another task management completion race
      https://git.kernel.org/mkp/scsi/c/5cb37a26355d

-- 
Martin K. Petersen	Oracle Linux Engineering