| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e8cc07cf-9bd1-41a4-bd46-44e18179154b@cybernetics.com>
Date: Wed, 24 Sep 2025 15:41:32 -0400
From: Tony Battersby <tonyb@...ernetics.com>
To: Dmitry Bogdanov <d.bogdanov@...ro.com>
Cc: Nilesh Javali <njavali@...vell.com>,
GR-QLogic-Storage-Upstream@...vell.com,
"James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
linux-scsi <linux-scsi@...r.kernel.org>, target-devel@...r.kernel.org,
scst-devel@...ts.sourceforge.net,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/15] scsi: qla2xxx: fix oops during cmd abort
On 9/11/25 10:21, Dmitry Bogdanov wrote:
> On Mon, Sep 08, 2025 at 02:58:06PM -0400, Tony Battersby wrote:
>> (target mode)
>>
>> There is a race between the following:
>>
>> CPU 1:
>> scst_hw_pending_work_fn() ->
>> sqa_on_hw_pending_cmd_timeout() ->
>> qlt_abort_cmd() ->
>> qlt_unmap_sg()
>>
>> CPU 2:
>> qla_do_work() ->
>> qla24xx_process_response_queue() ->
>> qlt_do_ctio_completion() ->
>> qlt_unmap_sg()
>>
>> Two CPUs calling qlt_unmap_sg() on the same cmd at the same time
>> results in an oops:
>>
>> dma_unmap_sg_attrs()
>> BUG_ON(!valid_dma_direction(dir));
>>
>> This race is more likely to happen because qlt_abort_cmd() may cause the
>> hardware to send a CTIO.
>>
>> The solution is to lock cmd->qpair->qp_lock_ptr when aborting a command.
>> This makes it possible to check the cmd state and make decisions about
>> what to do without racing with the CTIO handler and other code.
>>
>> - Lock cmd->qpair->qp_lock_ptr when aborting a cmd.
>> - Eliminate cmd->cmd_lock and change cmd->aborted back to a bitfield
>> since it is now protected by qp_lock_ptr just like all the other
>> flags.
>> - Add another command state QLA_TGT_STATE_DONE to avoid any possible
>> races between qlt_abort_cmd() and tgt_ops->free_cmd().
>> - Add the cmd->sent_term_exchg flag to indicate if
>> qlt_send_term_exchange() has already been called.
>> - For SCST (scst_hw_pending_work_fn()), export qlt_send_term_exchange()
>> and qlt_unmap_sg() so that they can be called directly instead of
>> trying to make qlt_abort_cmd() work for both HW timeout and TMR abort.
>> - Add TRC_CTIO_IGNORED for scst_hw_pending_work_fn().
>>
>> Fixes: 26f9ce53817a ("scsi: qla2xxx: Fix missed DMA unmap for aborted commands")
> You are trying to fix that commit using its approach, but actually that
> approach is the root cause itself. It is not ok to unmap dma while that
> memory is owned by HW.
>
> We use this patch 4 years already instead of 26f9ce53817a and never
> faced with such crashes.
>
>
> From: Dmitry Bogdanov <d.bogdanov@...ro.com>
> Date: Wed, 20 Oct 2021 15:57:31 +0300
> Subject: [PATCH] scsi: qla2xxx: clear cmds after chip reset
>
> Commands sent to FW, after chip reset got stuck and never freed as FW is
> not going to response to them anymore.
>
> This patch partially reverts aefed3e5548f at __qla2x00_abort_all_cmds.
>
> Fixes: aefed3e5548f ("scsi: qla2xxx: target: Fix offline port handling and host reset handling")
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@...ro.com>
Hey Dmitry, I want to pick up your patch and add it to my v2 patchset,
but I have made a few changes to it. Do I have your permission to add
your "Co-developed-by" and "Signed-off-by" tags to the patch below?
(Never did this before, I think I need to ask permission.)
Compared to your patch, I changed "if (cmd->se_cmd.t_state ==
TRANSPORT_WRITE_PENDING)" to "if (cmd->state ==
QLA_TGT_STATE_NEED_DATA)" (which is the way it was originally) to work
better with SCST and added the revert of 26f9ce53817a.
I will reply to this message with the two updated v2 patches that follow
your suggestions and remove the dangerous code that you objected to. If
you approve of them, then I will submit the entire v2 patchset, since
some of the other patches needed to be rebased.
Thanks for your reviews!
Tony
From: Tony Battersby <tonyb@...ernetics.com>
Subject: [PATCH] scsi: qla2xxx: clear cmds after chip reset
Commit aefed3e5548f ("scsi: qla2xxx: target: Fix offline port handling
and host reset handling") caused two problems:
1. Commands sent to FW, after chip reset got stuck and never freed as FW
is not going to respond to them anymore.
2. BUG_ON(cmd->sg_mapped) in qlt_free_cmd(). Commit 26f9ce53817a
("scsi: qla2xxx: Fix missed DMA unmap for aborted commands")
attempted to fix this, but introduced another bug under different
circumstances when two different CPUs were racing to call
qlt_unmap_sg() at the same time: BUG_ON(!valid_dma_direction(dir)) in
dma_unmap_sg_attrs().
So revert "scsi: qla2xxx: Fix missed DMA unmap for aborted commands" and
partially revert "scsi: qla2xxx: target: Fix offline port handling and
host reset handling" at __qla2x00_abort_all_cmds.
Fixes: aefed3e5548f ("scsi: qla2xxx: target: Fix offline port handling and host reset handling")
Fixes: 26f9ce53817a ("scsi: qla2xxx: Fix missed DMA unmap for aborted commands")
Signed-off-by: Tony Battersby <tonyb@...ernetics.com>
---
drivers/scsi/qla2xxx/qla_os.c | 20 ++++++++++++++++++--
drivers/scsi/qla2xxx/qla_target.c | 5 +----
drivers/scsi/qla2xxx/qla_target.h | 1 +
3 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index f0b77f13628d..739137ddfd68 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1875,10 +1875,26 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
continue;
}
cmd = (struct qla_tgt_cmd *)sp;
- cmd->aborted = 1;
+
+ if (cmd->sg_mapped)
+ qlt_unmap_sg(vha, cmd);
+
+ if (cmd->state == QLA_TGT_STATE_NEED_DATA) {
+ cmd->aborted = 1;
+ cmd->write_data_transferred = 0;
+ cmd->state = QLA_TGT_STATE_DATA_IN;
+ ha->tgt.tgt_ops->handle_data(cmd);
+ } else {
+ ha->tgt.tgt_ops->free_cmd(cmd);
+ }
break;
case TYPE_TGT_TMCMD:
- /* Skip task management functions. */
+ /*
+ * Currently, only ABTS response gets on the
+ * outstanding_cmds[]
+ */
+ ha->tgt.tgt_ops->free_mcmd(
+ (struct qla_tgt_mgmt_cmd *) sp);
break;
default:
break;
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index b700bfc642b3..2abdb8ce0302 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2447,7 +2447,7 @@ static int qlt_pci_map_calc_cnt(struct qla_tgt_prm *prm)
return -1;
}
-static void qlt_unmap_sg(struct scsi_qla_host *vha, struct qla_tgt_cmd *cmd)
+void qlt_unmap_sg(struct scsi_qla_host *vha, struct qla_tgt_cmd *cmd)
{
struct qla_hw_data *ha;
struct qla_qpair *qpair;
@@ -3795,9 +3795,6 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd)
spin_lock_irqsave(&cmd->cmd_lock, flags);
if (cmd->aborted) {
- if (cmd->sg_mapped)
- qlt_unmap_sg(vha, cmd);
-
spin_unlock_irqrestore(&cmd->cmd_lock, flags);
/*
* It's normal to see 2 calls in this path:
diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index 15a59c125c53..c483966d0a84 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -1058,6 +1058,7 @@ extern int qlt_abort_cmd(struct qla_tgt_cmd *);
extern void qlt_xmit_tm_rsp(struct qla_tgt_mgmt_cmd *);
extern void qlt_free_mcmd(struct qla_tgt_mgmt_cmd *);
extern void qlt_free_cmd(struct qla_tgt_cmd *cmd);
+extern void qlt_unmap_sg(struct scsi_qla_host *vha, struct qla_tgt_cmd *cmd);
extern void qlt_async_event(uint16_t, struct scsi_qla_host *, uint16_t *);
extern void qlt_enable_vha(struct scsi_qla_host *);
extern void qlt_vport_create(struct scsi_qla_host *, struct qla_hw_data *);
--
2.43.0
Powered by blists - more mailing lists