[PATCH v6 2/4] scsi: target: Only release caw_sem if this command took it

[Laurence Oberman <loberman@redhat.com>]

sbc_parse_cdb() installs compare_and_write_callback() as the command's
transport_complete_callback when it parses a COMPARE AND WRITE. The
semaphore that callback releases is not taken until much later, in
sbc_compare_and_write(), which runs when the command is executed. A
command that is aborted or fails between those two points reaches the
callback without holding dev->caw_sem, and releases it anyway.

compare_and_write_callback() tries to cover that case by returning early
when cmd->t_data_sg or cmd->t_bidi_data_sg is NULL, on the assumption that
a command with no buffers cannot have taken the semaphore. That assumption
does not hold: transport_generic_new_cmd() allocates both lists, and a
command aborted while it sits in TRANSPORT_WRITE_PENDING has the buffers
and not the semaphore. up() on a semaphore that was never taken raises its
count, which later lets two COMPARE AND WRITE commands run against the
same device at once.

The same callback can also run more than once for one command --
target_cmd_interrupted() and transport_generic_request_failure() both
invoke it -- which releases the semaphore a second time.

Track whether the semaphore is held in SCF_CAW_SEM_HELD, set once
down_interruptible() has succeeded in sbc_compare_and_write(). Release it
through sbc_caw_sem_release(), which tests and clears the flag under
t_state_lock and only calls up() if the flag was set, so neither a command
that never took the semaphore nor a second invocation of the callback can
release it. The early return for missing buffers becomes a goto out, since
the release is now decided by the flag rather than inferred from the
buffers.

Cc: stable@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    No Fixes: tag: the callback has been installed at parse time and the
    semaphore taken at execute time since COMPARE AND WRITE was added, so I
    am not confident which commit to name.  Please say which you would like
    and I will add it.  Cc: stable is on the patch because the consequence --
    a semaphore released without being taken -- is worth backporting
    regardless of which commit is cited.
    
    Not observed in the field; found while reviewing the double completion
    that patch 2 fixes.

 drivers/target/target_core_sbc.c  | 49 +++++++++++++++++++++----------
 include/target/target_core_base.h |  1 +
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 8c52e1e..5193453 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -347,10 +347,33 @@ sbc_execute_rw(struct se_cmd *cmd)
 			       cmd->data_direction);
 }
 
+/*
+ * Release dev->caw_sem, but only if this command is the one holding it.
+ * sbc_compare_and_write() takes the semaphore when the command is executed,
+ * which is long after sbc_parse_cdb() installs the completion callback, so a
+ * command that is aborted or fails before then reaches the callback without
+ * holding it.  The test and clear are done under t_state_lock so that a second
+ * invocation of the callback cannot release the semaphore twice.
+ */
+static void sbc_caw_sem_release(struct se_cmd *cmd)
+	__must_not_hold(&cmd->t_state_lock)
+{
+	struct se_device *dev = cmd->se_dev;
+	unsigned long flags;
+	bool held;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	held = cmd->se_cmd_flags & SCF_CAW_SEM_HELD;
+	cmd->se_cmd_flags &= ~SCF_CAW_SEM_HELD;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+	if (held)
+		up(&dev->caw_sem);
+}
+
 static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
 					     int *post_ret)
 {
-	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret = TCM_NO_SENSE;
 	unsigned long flags;
 
@@ -363,11 +386,7 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
 	}
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-	/*
-	 * Unlock ->caw_sem originally obtained during sbc_compare_and_write()
-	 * before the original READ I/O submission.
-	 */
-	up(&dev->caw_sem);
+	sbc_caw_sem_release(cmd);
 
 	return ret;
 }
@@ -450,11 +469,12 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 
 	if (!success) {
 		/*
-		 * Handle early failure in transport_generic_request_failure(),
-		 * which will not have taken ->caw_sem yet..
+		 * Handle early failure in transport_generic_request_failure().
+		 * Whether ->caw_sem was taken is tracked in SCF_CAW_SEM_HELD, so
+		 * this is only about there being no buffers to compare.
 		 */
 		if (!cmd->t_data_sg || !cmd->t_bidi_data_sg)
-			return TCM_NO_SENSE;
+			goto out;
 
 		/*
 		 * The command has been stopped or aborted so
@@ -556,11 +576,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 	return ret;
 
 out:
-	/*
-	 * In the MISCOMPARE or failure case, unlock ->caw_sem obtained in
-	 * sbc_compare_and_write() before the original READ I/O submission.
-	 */
-	up(&dev->caw_sem);
+	sbc_caw_sem_release(cmd);
 	sg_free_table(&write_tbl);
 	return ret;
 }
@@ -581,6 +597,9 @@ sbc_compare_and_write(struct se_cmd *cmd)
 		cmd->transport_complete_callback = NULL;
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
+	spin_lock_irq(&cmd->t_state_lock);
+	cmd->se_cmd_flags |= SCF_CAW_SEM_HELD;
+	spin_unlock_irq(&cmd->t_state_lock);
 	/*
 	 * Reset cmd->data_length to individual block_size in order to not
 	 * confuse backend drivers that depend on this value matching the
@@ -592,7 +611,7 @@ sbc_compare_and_write(struct se_cmd *cmd)
 			      DMA_FROM_DEVICE);
 	if (ret) {
 		cmd->transport_complete_callback = NULL;
-		up(&dev->caw_sem);
+		sbc_caw_sem_release(cmd);
 		return ret;
 	}
 	/*
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 002b0fc..bd846ab 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -168,6 +168,7 @@ enum se_cmd_flags_table {
 	SCF_TREAT_READ_AS_NORMAL		= (1 << 18),
 	SCF_TASK_ORDERED_SYNC			= (1 << 19),
 	SCF_ATOMIC				= (1 << 20),
+	SCF_CAW_SEM_HELD			= (1 << 21),
 };
 
 /*
-- 
2.43.0