[PATCH v5 1/2] scsi: target: Complete an aborted command at most once

[Laurence Oberman]

target_cmd_interrupted() queues target_abort_work for a command that has
CMD_T_ABORTED set and returns without setting CMD_T_COMPLETE and without
changing t_state, so target core records nothing that would stop a second
completion of the same command. A second call re-initialises and re-queues
cmd->work while the first is still queued or running, and calls
transport_complete_callback() again. For COMPARE AND WRITE that callback is
compare_and_write_callback(), which releases dev->caw_sem, so a repeated
call releases that semaphore twice.

More than one site can reach that conclusion for the same command. A WRITE
still in TRANSPORT_WRITE_PENDING has never been passed to the backend, so
the fabric has to complete it when a TMR aborts it, and for iSCSI both the
DataOut paths and iscsit_release_commands_from_conn() during connection
teardown will do so.

Guard the CMD_T_ABORTED branch with a one-shot CMD_T_ABORT_QUEUED flag,
tested and set under t_state_lock. The CMD_T_ABORTED test moves under that
lock as well, which is the lock __target_check_io_state() sets
CMD_T_ABORTED under, so there is no window between the test and the set.
Second and later calls are a no-op by design and still return true, so
callers do not proceed with normal completion. A fabric that completes an
aborted command exactly once never reaches the guard and is unaffected.

The CMD_T_STOP branch is deliberately left unguarded. complete_all() is
idempotent and t_transport_stop_comp is initialised once in
__target_init_cmd(), so a repeated call there is harmless. A guard shared
between the two branches would also be wrong: a first call taking the
CMD_T_STOP branch would then suppress the queueing of target_abort_work if
CMD_T_ABORTED were set afterwards, which is the hang this series started
from.

Suggested-by: Bart Van Assche <bvanassche@xxxxxxx>
Signed-off-by: Laurence Oberman <loberman@xxxxxxxxxx>
---

Notes:
    Prerequisite for patch 2, which relies on a second completion of an
    aborted command being a no-op.  Both need to travel together to stable.
    
    I have not put a Fixes: tag on this one -- the double completion is older
    than the iSCSI hang and I am not certain which commit to name.  If you
    would like one here, please say which.

 drivers/target/target_core_transport.c | 37 ++++++++++++++++++++------
 include/target/target_core_base.h      |  1 +
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 927861c..4896a92 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -882,24 +882,45 @@ static void target_abort_work(struct work_struct *work)
 	target_handle_abort(cmd);
 }
 
+/*
+ * Returns true if the command has been aborted or stopped, in which case the
+ * caller must not carry on with normal completion.  For an aborted command the
+ * work that must not run twice -- the completion callback and the queueing of
+ * target_abort_work -- is done once, under CMD_T_ABORT_QUEUED.
+ */
 static bool target_cmd_interrupted(struct se_cmd *cmd)
 {
+	bool queue_abort = false, stop = false;
+	unsigned long flags;
 	int post_ret;
 
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	if (cmd->transport_state & CMD_T_ABORTED) {
-		if (cmd->transport_complete_callback)
-			cmd->transport_complete_callback(cmd, false, &post_ret);
+		if (cmd->transport_state & CMD_T_ABORT_QUEUED) {
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+			return true;
+		}
+		cmd->transport_state |= CMD_T_ABORT_QUEUED;
+		queue_abort = true;
+	} else if (cmd->transport_state & CMD_T_STOP) {
+		stop = true;
+	}
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+	if (!queue_abort && !stop)
+		return false;
+
+	if (cmd->transport_complete_callback)
+		cmd->transport_complete_callback(cmd, false, &post_ret);
+
+	if (queue_abort) {
 		INIT_WORK(&cmd->work, target_abort_work);
 		queue_work(target_completion_wq, &cmd->work);
-		return true;
-	} else if (cmd->transport_state & CMD_T_STOP) {
-		if (cmd->transport_complete_callback)
-			cmd->transport_complete_callback(cmd, false, &post_ret);
+	} else {
 		complete_all(&cmd->t_transport_stop_comp);
-		return true;
 	}
 
-	return false;
+	return true;
 }
 
 static void target_complete(struct se_cmd *cmd, int success)
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 002b0fc..7fa6733 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -537,6 +537,7 @@ struct se_cmd {
 #define CMD_T_STOP		(1 << 5)
 #define CMD_T_TAS		(1 << 10)
 #define CMD_T_FABRIC_STOP	(1 << 11)
+#define CMD_T_ABORT_QUEUED	(1 << 12)
 	spinlock_t		t_state_lock;
 	struct kref		cmd_kref;
 	struct completion	t_transport_stop_comp;
-- 
2.43.0