Re: [PATCH 0/2] target: iscsi: fix hanged WRITE_PENDING commands

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

Hi Laurence,

On Tue, Oct 27, 2026 at 04:08:08PM -0400, Laurence Oberman wrote:
> 
> On Tue, 2026-10-27 at 21:01 +0300, Dmitry Bogdanov wrote:
> > ISCSI has some old leftovers in the code that prevents passing the
> > aborted
> > command to the target core to make the actual abort. Such commands
> > hang
> > until connection is closed.
> > So remove those leftovers.
> >
> > Dmitry Bogdanov (2):
> >   Revert "scsi: target: iscsi: Fix hang for aborted WRITE_PENDING
> >     commands"
> >   scsi: target: iscsi: fix hanging WRITE_PENDING commands
> >
> >  drivers/target/iscsi/iscsi_target.c      | 19 -------------------
> >  drivers/target/iscsi/iscsi_target_erl1.c |  8 --------
> >  2 files changed, 27 deletions(-)
> 
> Hello Dmitri
> Thanks for posting this. I have a series in flight for the same hang:
> 
> https://lore.kernel.org/linux-scsi/20261027122837.2148074-1-loberman@redhat.com/

I know, that is why both you and Maurizio in To: field. I want from
you some comments. Thank you for review.

> 
> Is the revert maybe aimed at the wrong layer, and the sashiko review of
> your 2/2 makes the case better than I can: with the fabric check
> removed, an aborted command reaches target_execute_cmd(),
> target_cmd_interrupted() queues target_abort_work and returns without
> changing t_state, the command stays on conn_cmd_list in
> TRANSPORT_WRITE_PENDING, and iscsit_release_commands_from_conn()
> completes it a second time if the connection closes.

As I said in response to sashiko-bot, that is the current (always be) behavior.
It is not introduced by my patch.

> That is the same double completion you are reverting d5869dae5080 for.
> It
> is not a defect in Maurizio's patch and it will not be fixed by
> replacing
> it -- target_cmd_interrupted() records nothing when it queues an abort,
> so
> any site that completes an aborted WRITE_PENDING command can be
> followed
> by another one. I found this from vmcores on a customer system before
> either of us posted, so no argument from me that it is real.

Not the same, d5869dae5080 greatly increases the probability of the
double completion by calling target_complete_cmd() which is allowed to be
called only once for the cmd. Calling it twice is wrong by nature. Fabric
module shall call either target_execute_cmd() either
transport_generic_request_failure(). target_complete_cmd() is for
backstore modules. That target_complete_cmd() at the iscsi
connection release is not so good too. But it was intended to be the
last(and the only) call from fabric driver for the command never be in
the target core.

> 
> Patch 3 of my series fixes it there, under t_state_lock, for both
> queueing sites in target_core_transport.c and for every fabric. With
> that
> in place your 2/2 is safe and d5869dae5080 does not need reverting for
> this reason -- which matters, because the revert is not free: that
> commit
> carries CVE-2026-97951 and is in 5.10.y, 5.15.y, 6.1.y, 6.6.y, 6.12.y,
> 6.18.y and 7.2.y.

My intention is to not create a situation when there a race is possible
instead of fixing those races. 

> Two questions on 2/2 independent of all that.
> 
> Removing the sense_reason early return in iscsit_execute_cmd() means
> transport_send_check_condition_and_sense() now runs for an aborted
> command. It guards only on SCF_SENT_CHECK_CONDITION, never checks
> CMD_T_ABORTED, and ends in queue_status(); target_handle_abort() also
> calls queue_status() for a CMD_T_TAS command, and each drops a
> reference.
> Can an aborted command with a sense reason send both a CHECK CONDITION
> and a TASK ABORTED, and drop two references?

No, transport_send_check_condition_and_sense will change t_state, send
status and decrease cmd->refcnt and then unblock wait_for_completion of
ABORT/LUN_RESET. Both the commnand and ABORT of that command will be
completed sucessfully.
Noone calls target_handle_abort().

> And with the early return gone, a LUN_RESET aborting a large WRITE
> waits in target_put_cmd_and_wait() for the whole remaining data
> transfer. That is bounded where today it is not, but on a large
> transfer it may still trip the hung task timeout that made this
> visible.
 
That is a idea for a future discussion and improvmens. This patchset is
narrowed to just to fix the stuck of WRITE_PENDING commands.

The abort of WRITE_PENDIG command in the state waiting for DataOuts
currently will wait for the data completion. So, my patch again does not
do worse.

> On the overlap: your 2/2 and patch 4 of my series touch the same lines,
> so only one can apply, and I have no attachment to mine if yours is the
> better shape. Patches 1 to 3 are core-side and conflict with nothing
> you have posted.
> 
> Martin, would you prefer those three as their own series,
> given both candidate iSCSI fixes need patch 3 to be correct?
> 
> I am happy to test either or both: I have a reporter's environment that
> reproduces this reliably and 300-cycle runs on it.

My patch is more than 2 years old in customers setups, but certainly
it would be great to test it in other environments too.

BR,
 Dmitry