Re: [PATCH 00/16] scsi: libsas and users: Factor out LLDD TMF code
From: John Garry
Date: Fri Feb 04 2022 - 06:51:00 EST
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@xxxxxxxxxx/
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,
JohnFrom 1aea5d406de8806de227f79c2728899959153400 Mon Sep 17 00:00:00 2001
From: John Garry <john.garry@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>
Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
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