linux-kernel - Re: [DMARC Error]Re: [PATCH 08/15] scsi: qla2xxx: fix oops during cmd abort

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250925084252.GA821@yadro.com>
Date: Thu, 25 Sep 2025 11:42:52 +0300
From: Dmitry Bogdanov <d.bogdanov@...ro.com>
To: Tony Battersby <tonyb@...ernetics.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: [DMARC Error]Re: [PATCH 08/15] scsi: qla2xxx: fix oops during
 cmd abort

On Wed, Sep 24, 2025 at 03:41:32PM -0400, Tony Battersby wrote:
> 
> 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.)

Yes, of course, feel free to use it. Signed-off-by is enough I think.

 
> 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.

Yes, that is more correct.
 
> 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.
>