Re: [PATCH 00/16] scsi: libsas and users: Factor out LLDD TMF code

[John Garry <john.garry@huawei.com>]

[-- Attachment #1: Type: text/plain, Size: 5092 bytes --]

On 04/02/2022 11:27, Damien Le Moal wrote:
>> As I mentioned, having this fail is a red flag. If I was pushed to guess
>> what has happened, I'd say the FW is faulting due to some erroneous
>> driver behaviour.
> I am still thinking that there is something wrong with the handling of
> NCQ NON DATA command. There are several places where the code determines
> non-data vs pio vs dma vs fpdma (ncq), and NCQ NON DATA always falls in
> the fpdma bucket, which is wrong.
> 

Ok, I will have a look at this. We made some libsas changes related to 
this not so long ago to "fix" something, see:

53de092f47f ("scsi: libsas: Set data_dir as DMA_NONE if libata marks qc 
as NODATA")

176ddd89171d ("scsi: libsas: Reset num_scatter if libata marks qc as 
NODATA")

>>> This is the submission path, not completion. The code is:
>>>
>>> (gdb) list *(pm8001_queue_command+0x842)
>>> 0x3d42 is in pm8001_queue_command (drivers/scsi/pm8001/pm8001_sas.c:491).
>>> 486				atomic_dec(&pm8001_dev->running_req);
>>> 487				goto err_out_tag;
>>> 488			}
>>> 489			/* TODO: select normal or high priority */
>>> 490			spin_lock(&t->task_state_lock);
>>> 491			t->task_state_flags |= SAS_TASK_AT_INITIATOR;
>>> 492			spin_unlock(&t->task_state_lock);
>>> 493		} while (0);
>>> 494		rc = 0;
>>> 495		goto out_done;
>>>
>>> So the task is already completed when the submission path tries to set
>>> the state flag ? Debugging...
>> Yeah, that's how it looks.
>>
>> I already mentioned this problem here:
>>
>> https://lore.kernel.org/linux-scsi/0cc0c435-b4f2-9c76-258d-865ba50a29dd@huawei.com/
>>
>> Maybe we should just fix it now to rule it out of possibly causing other
>> issues... I was reluctant to fix it as many places seems to need to be
>> touched. Let me check it.
> Here is my current fix:
> 
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 1b95c73d12d1..16c101577dd3 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -453,13 +453,18 @@ int pm8001_queue_command(struct sas_task *task,
> gfp_t gfp_flags)
>                  ccb->ccb_tag = tag;
>                  ccb->task = t;
>                  ccb->device = pm8001_dev;
> +
> +               /* TODO: select normal or high priority */
> +               atomic_inc(&pm8001_dev->running_req);
> +               spin_lock(&t->task_state_lock);
> +               t->task_state_flags |= SAS_TASK_AT_INITIATOR;
> +               spin_unlock(&t->task_state_lock);
> +
>                  switch (task_proto) {
>                  case SAS_PROTOCOL_SMP:
> -                       atomic_inc(&pm8001_dev->running_req);
>                          rc = pm8001_task_prep_smp(pm8001_ha, ccb);
>                          break;
>                  case SAS_PROTOCOL_SSP:
> -                       atomic_inc(&pm8001_dev->running_req);
>                          if (is_tmf)
>                                  rc = pm8001_task_prep_ssp_tm(pm8001_ha,
>                                          ccb, tmf);
> @@ -468,7 +473,6 @@ int pm8001_queue_command(struct sas_task *task,
> gfp_t gfp_flags)
>                          break;
>                  case SAS_PROTOCOL_SATA:
>                  case SAS_PROTOCOL_STP:
> -                       atomic_inc(&pm8001_dev->running_req);
>                          rc = pm8001_task_prep_ata(pm8001_ha, ccb);
>                          break;
>                  default:
> @@ -480,13 +484,12 @@ int pm8001_queue_command(struct sas_task *task,
> gfp_t gfp_flags)
> 
>                  if (rc) {
>                          pm8001_dbg(pm8001_ha, IO, "rc is %x\n", rc);
> +                       spin_lock(&t->task_state_lock);
> +                       t->task_state_flags &= ~SAS_TASK_AT_INITIATOR;
> +                       spin_unlock(&t->task_state_lock);
>                          atomic_dec(&pm8001_dev->running_req);
>                          goto err_out_tag;
>                  }
> -               /* TODO: select normal or high priority */
> -               spin_lock(&t->task_state_lock);
> -               t->task_state_flags |= SAS_TASK_AT_INITIATOR;
> -               spin_unlock(&t->task_state_lock);
>          } while (0);
>          rc = 0;
>          goto out_done;
> 
> With this, No KASAN complaint. I will send a proper patch ASAP.

I had just prepared a patch, but different, attached.

> 
> Of note is that I cannot see what the flag SAS_TASK_AT_INITIATOR is for.
> It is set and unset only, never tested anywhere in libsas nor pm8001
> driver. This flag seems totally useless to me, unless this is something
> that the HW can see ?

Right, it is only checked by isci AFAIC. And it is not something which 
HW can see.

Since libsas does not check it the semantics are ill-defined. However I 
think that it's worth setting it completeness; I thought it was better 
setting it just before the command is delivered to HW, but the 
implementation touches more SW. Simpler approach may be better since it 
aligns with how this flag may be cleared in pm8001_chip_sata_req() under 
asusmption it is already set.

Thanks,
John

[-- Attachment #2: 0001-scsi-pm8001-Set-SAS_TASK_AT_INITIATOR-before-deliver.patch --]
[-- Type: text/plain, Size: 6173 bytes --]

From 1aea5d406de8806de227f79c2728899959153400 Mon Sep 17 00:00:00 2001
From: John Garry <john.garry@huawei.com>
Date: Fri, 4 Feb 2022 11:12:12 +0000
Subject: [PATCH] scsi: pm8001: Set SAS_TASK_AT_INITIATOR before delivering
 task

Both Damien and I have seen a KASAN use-after-free warn for accessing a
sas_task after delivering the associated command to HW.

The issue is that once the command is delivered it is completed (and freed)
async, and, as such, it is not safe to touch the sas_task again in the
delivery path.

However the sas_task is touched again in setting the task state
SAS_TASK_AT_INITIATOR flag. The semantics for that flag is not defined,
specifically because it is not checked by libsas. Indeed, it is not even
checked by this driver. For the sake of completeness, set that flag before
delivering the command to the HW.

Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index c814e5071712..c6f5aa27bfbf 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -1796,6 +1796,7 @@ static void pm8001_send_abort_all(struct pm8001_hba_info *pm8001_ha,
 	task_abort.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
 	task_abort.tag = cpu_to_le32(ccb_tag);
 
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &task_abort,
 			sizeof(task_abort), 0);
 	if (ret)
@@ -1868,6 +1869,7 @@ static void pm8001_send_read_log(struct pm8001_hba_info *pm8001_ha,
 	sata_cmd.ncqtag_atap_dir_m |= ((0x1 << 7) | (0x5 << 9));
 	memcpy(&sata_cmd.sata_fis, &fis, sizeof(struct host_to_dev_fis));
 
+	pm8001_set_task_at_initiator(task);
 	res = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &sata_cmd,
 			sizeof(sata_cmd), 0);
 	if (res) {
@@ -4198,6 +4200,8 @@ static int pm8001_chip_smp_req(struct pm8001_hba_info *pm8001_ha,
 	smp_cmd.long_smp_req.long_resp_size =
 		cpu_to_le32((u32)sg_dma_len(&task->smp_task.smp_resp)-4);
 	build_smp_cmd(pm8001_dev->device_id, smp_cmd.tag, &smp_cmd);
+
+	pm8001_set_task_at_initiator(task);
 	rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
 			&smp_cmd, sizeof(smp_cmd), 0);
 	if (rc)
@@ -4266,6 +4270,7 @@ static int pm8001_chip_ssp_io_req(struct pm8001_hba_info *pm8001_ha,
 		ssp_cmd.len = cpu_to_le32(task->total_xfer_len);
 		ssp_cmd.esgl = 0;
 	}
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &ssp_cmd,
 			sizeof(ssp_cmd), 0);
 	return ret;
@@ -4375,6 +4380,7 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 		}
 	}
 
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &sata_cmd,
 			sizeof(sata_cmd), 0);
 	return ret;
@@ -4646,6 +4652,7 @@ int pm8001_chip_ssp_tm_req(struct pm8001_hba_info *pm8001_ha,
 	if (pm8001_ha->chip_id != chip_8001)
 		sspTMCmd.ds_ads_m = 0x08;
 	circularQ = &pm8001_ha->inbnd_q_tbl[0];
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &sspTMCmd,
 			sizeof(sspTMCmd), 0);
 	return ret;
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index cec1ad47bf5b..104fb4fe3526 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -488,9 +488,6 @@ static int pm8001_task_exec(struct sas_task *task,
 		}
 		mdelay(10);
 		/* TODO: select normal or high priority */
-		spin_lock(&t->task_state_lock);
-		t->task_state_flags |= SAS_TASK_AT_INITIATOR;
-		spin_unlock(&t->task_state_lock);
 	} while (0);
 	rc = 0;
 	goto out_done;
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index a17da1cebce1..a7da0cb35a77 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -748,5 +748,12 @@ pm8001_ccb_task_free_done(struct pm8001_hba_info *pm8001_ha,
 	task->task_done(task);
 }
 
+static inline void pm8001_set_task_at_initiator(struct sas_task *t)
+{
+	spin_lock(&t->task_state_lock);
+	t->task_state_flags |= SAS_TASK_AT_INITIATOR;
+	spin_unlock(&t->task_state_lock);
+}
+
 #endif
 
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 2530d1365556..11b911fac2c8 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -1809,6 +1809,7 @@ static void pm80xx_send_abort_all(struct pm8001_hba_info *pm8001_ha,
 	task_abort.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
 	task_abort.tag = cpu_to_le32(ccb_tag);
 
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &task_abort,
 			sizeof(task_abort), 0);
 	pm8001_dbg(pm8001_ha, FAIL, "Executing abort task end\n");
@@ -1885,6 +1886,7 @@ static void pm80xx_send_read_log(struct pm8001_hba_info *pm8001_ha,
 	sata_cmd.ncqtag_atap_dir_m_dad |= ((0x1 << 7) | (0x5 << 9));
 	memcpy(&sata_cmd.sata_fis, &fis, sizeof(struct host_to_dev_fis));
 
+	pm8001_set_task_at_initiator(task);
 	res = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &sata_cmd,
 			sizeof(sata_cmd), 0);
 	pm8001_dbg(pm8001_ha, FAIL, "Executing read log end\n");
@@ -4342,6 +4344,7 @@ static int pm80xx_chip_smp_req(struct pm8001_hba_info *pm8001_ha,
 	kunmap_atomic(to);
 	build_smp_cmd(pm8001_dev->device_id, smp_cmd.tag,
 				&smp_cmd, pm8001_ha->smp_exp_mode, length);
+	pm8001_set_task_at_initiator(task);
 	rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &smp_cmd,
 			sizeof(smp_cmd), 0);
 	if (rc)
@@ -4538,6 +4541,7 @@ static int pm80xx_chip_ssp_io_req(struct pm8001_hba_info *pm8001_ha,
 			ssp_cmd.esgl = 0;
 		}
 	}
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
 			&ssp_cmd, sizeof(ssp_cmd), q_index);
 	return ret;
@@ -4774,6 +4778,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 				ccb->ccb_tag, opc,
 				qc ? qc->tf.command : 0, // ata opcode
 				ccb->device ? atomic_read(&ccb->device->running_req) : 0);
+	pm8001_set_task_at_initiator(task);
 	ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
 			&sata_cmd, sizeof(sata_cmd), q_index);
 	return ret;
-- 
2.26.2