[PATCH 0/5] target task management fixes and cleanups

[PATCH 0/5] target task management fixes and cleanups

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Hi Nic,

A few fixes for TMR handling (fix crashes when a backend is really
slow, fix a reference leak if we get a TMR for a non-existent LUN) and
a couple of trivial cleanups in related code.

Roland Dreier (5):
  target: Don't let abort handling free pending write commands too soon
  target: Fix use-after-free in LUN RESET handling
  target: Release se_cmd when LUN lookup fails for TMR
  target: Remove useless if statement
  target: Remove never-used TMR_FABRIC_TMR enum value

 drivers/target/target_core_tmr.c       | 12 ------------
 drivers/target/target_core_transport.c |  8 +++++---
 include/target/target_core_base.h      |  1 -
 3 files changed, 5 insertions(+), 16 deletions(-)

-- 
1.8.0

[PATCH 1/5] target: Don't let abort handling free pending write commands too soon

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

The following sequence happens for write commands (or any other
commands with a data out phase):

 - The transport calls target_submit_cmd(), which sets CMD_T_ACTIVE in
   cmd->transport_state and sets cmd->t_state to TRANSPORT_NEW_CMD.
 - Things go on transport_generic_new_cmd(), which notices that the
   command needs to transfer data, so it sets cmd->t_state to
   TRANSPORT_WRITE_PENDING and calls transport_cmd_check_stop().
 - transport_cmd_check_stop() clears CMD_T_ACTIVE in cmd->transport_state
   and returns in the normal case.
 - Then we continue on to call ->se_tfo->write_pending().
 - The data comes back from the initiator, and the transport calls
   target_execute_cmd(), which sets cmd->t_state to TRANSPORT_PROCESSING
   and calls into the backend to actually write the data.

At this point, the backend might take a long time to complete the
command, since it has to do real IO.  If an abort request comes in for
this command at this point, it will not wait for the command to finish
since CMD_T_ACTIVE is not set.  Then when the command does finally
finish, we blow up with use-after-free.

Avoid this by setting CMD_T_ACTIVE in target_execute_cmd() so that
transport_wait_for_tasks() waits for the command to finish executing.
This matches the behavior from before commit 1389533ef944 ("target:
remove transport_generic_handle_data"), when data was signaled via
transport_generic_handle_data(), which set CMD_T_ACTIVE because it
called transport_add_cmd_to_queue().

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c23c76c..1dd9d66 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1688,6 +1688,7 @@ void target_execute_cmd(struct se_cmd *cmd)
 	}
 
 	cmd->t_state = TRANSPORT_PROCESSING;
+	cmd->transport_state |= CMD_T_ACTIVE;
 	spin_unlock_irq(&cmd->t_state_lock);
 
 	if (!target_handle_task_attr(cmd))
-- 
1.8.0

Re: [PATCH 1/5] target: Don't let abort handling free pending write commands too soon

[Nicholas A. Bellinger]

On Wed, 2013-01-02 at 12:47 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> The following sequence happens for write commands (or any other
> commands with a data out phase):
> 
>  - The transport calls target_submit_cmd(), which sets CMD_T_ACTIVE in
>    cmd->transport_state and sets cmd->t_state to TRANSPORT_NEW_CMD.
>  - Things go on transport_generic_new_cmd(), which notices that the
>    command needs to transfer data, so it sets cmd->t_state to
>    TRANSPORT_WRITE_PENDING and calls transport_cmd_check_stop().
>  - transport_cmd_check_stop() clears CMD_T_ACTIVE in cmd->transport_state
>    and returns in the normal case.
>  - Then we continue on to call ->se_tfo->write_pending().
>  - The data comes back from the initiator, and the transport calls
>    target_execute_cmd(), which sets cmd->t_state to TRANSPORT_PROCESSING
>    and calls into the backend to actually write the data.
> 
> At this point, the backend might take a long time to complete the
> command, since it has to do real IO.  If an abort request comes in for
> this command at this point, it will not wait for the command to finish
> since CMD_T_ACTIVE is not set.  Then when the command does finally
> finish, we blow up with use-after-free.
> 
> Avoid this by setting CMD_T_ACTIVE in target_execute_cmd() so that
> transport_wait_for_tasks() waits for the command to finish executing.
> This matches the behavior from before commit 1389533ef944 ("target:
> remove transport_generic_handle_data"), when data was signaled via
> transport_generic_handle_data(), which set CMD_T_ACTIVE because it
> called transport_add_cmd_to_queue().
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---

Applied a CC' to stable with a extra comment wrt to the commit that
introduced this regression.

Thanks again Roland!

--nab

>  drivers/target/target_core_transport.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index c23c76c..1dd9d66 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1688,6 +1688,7 @@ void target_execute_cmd(struct se_cmd *cmd)
>  	}
>  
>  	cmd->t_state = TRANSPORT_PROCESSING;
> +	cmd->transport_state |= CMD_T_ACTIVE;
>  	spin_unlock_irq(&cmd->t_state_lock);
>  
>  	if (!target_handle_task_attr(cmd))

[PATCH 2/5] target: Fix use-after-free in LUN RESET handling

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

If a backend IO takes a really long then an initiator might abort a
command, and then when it gives up on the abort, send a LUN reset too,
all before we process any of the original command or the abort.  (The
abort will wait for the backend IO to complete too)

When the backend IO final completes (or fails), the abort handling
will proceed and queue up a "return aborted status" operation.  Then,
while that's still pending, the LUN reset might find the original
command still on the LUN's list of commands and try to return aborted
status again, which leads to a use-after free when the first
se_tfo->queue_status call frees the command and then the second
se_tfo->queue_status call runs.

Fix this by removing a command from the LUN state_list when we first
are about to queue aborted status; we shouldn't do anything
LUN-related after we've started returning status, so this seems like
the correct thing to do.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 1dd9d66..49390d8 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -541,9 +541,6 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
 
 void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 {
-	if (!(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
-		transport_lun_remove_cmd(cmd);
-
 	if (transport_cmd_check_stop_to_fabric(cmd))
 		return;
 	if (remove)
@@ -2805,6 +2802,8 @@ void transport_send_task_abort(struct se_cmd *cmd)
 	}
 	cmd->scsi_status = SAM_STAT_TASK_ABORTED;
 
+	transport_lun_remove_cmd(cmd);
+
 	pr_debug("Setting SAM_STAT_TASK_ABORTED status for CDB: 0x%02x,"
 		" ITT: 0x%08x\n", cmd->t_task_cdb[0],
 		cmd->se_tfo->get_task_tag(cmd));
-- 
1.8.0

Re: [PATCH 2/5] target: Fix use-after-free in LUN RESET handling

[Nicholas A. Bellinger]

On Wed, 2013-01-02 at 12:47 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> If a backend IO takes a really long then an initiator might abort a
> command, and then when it gives up on the abort, send a LUN reset too,
> all before we process any of the original command or the abort.  (The
> abort will wait for the backend IO to complete too)
> 
> When the backend IO final completes (or fails), the abort handling
> will proceed and queue up a "return aborted status" operation.  Then,
> while that's still pending, the LUN reset might find the original
> command still on the LUN's list of commands and try to return aborted
> status again, which leads to a use-after free when the first
> se_tfo->queue_status call frees the command and then the second
> se_tfo->queue_status call runs.
> 
> Fix this by removing a command from the LUN state_list when we first
> are about to queue aborted status; we shouldn't do anything
> LUN-related after we've started returning status, so this seems like
> the correct thing to do.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---

Nice catch on this TMR corner case w/ long out-standing I/O.

Applied to target-pending/master with a CC' to stable.

--nab

>  drivers/target/target_core_transport.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 1dd9d66..49390d8 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -541,9 +541,6 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
>  
>  void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
>  {
> -	if (!(cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
> -		transport_lun_remove_cmd(cmd);
> -
>  	if (transport_cmd_check_stop_to_fabric(cmd))
>  		return;
>  	if (remove)
> @@ -2805,6 +2802,8 @@ void transport_send_task_abort(struct se_cmd *cmd)
>  	}
>  	cmd->scsi_status = SAM_STAT_TASK_ABORTED;
>  
> +	transport_lun_remove_cmd(cmd);
> +
>  	pr_debug("Setting SAM_STAT_TASK_ABORTED status for CDB: 0x%02x,"
>  		" ITT: 0x%08x\n", cmd->t_task_cdb[0],
>  		cmd->se_tfo->get_task_tag(cmd));

[PATCH 3/5] target: Release se_cmd when LUN lookup fails for TMR

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

When transport_lookup_tmr_lun() fails and we return a task management
response from target_complete_tmr_failure(), we need to call
transport_cmd_check_stop_to_fabric() to release the last ref to the
cmd after calling se_tfo->queue_tm_rsp(), or else we will never remove
the failed TMR from the session command list (and we'll end up waiting
forever when trying to tear down the session).

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 49390d8..fe6208f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1393,6 +1393,8 @@ static void target_complete_tmr_failure(struct work_struct *work)
 
 	se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
 	se_cmd->se_tfo->queue_tm_rsp(se_cmd);
+
+	transport_cmd_check_stop_to_fabric(cmd);
 }
 
 /**
-- 
1.8.0

Re: [PATCH 3/5] target: Release se_cmd when LUN lookup fails for TMR

[Nicholas A. Bellinger]

On Wed, 2013-01-02 at 12:47 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> When transport_lookup_tmr_lun() fails and we return a task management
> response from target_complete_tmr_failure(), we need to call
> transport_cmd_check_stop_to_fabric() to release the last ref to the
> cmd after calling se_tfo->queue_tm_rsp(), or else we will never remove
> the failed TMR from the session command list (and we'll end up waiting
> forever when trying to tear down the session).
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/target/target_core_transport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 49390d8..fe6208f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1393,6 +1393,8 @@ static void target_complete_tmr_failure(struct work_struct *work)
>  
>  	se_cmd->se_tmr_req->response = TMR_LUN_DOES_NOT_EXIST;
>  	se_cmd->se_tfo->queue_tm_rsp(se_cmd);
> +
> +	transport_cmd_check_stop_to_fabric(cmd);
>  }
>  
>  /**

Looks good.  CC'ing to stable as well.

--nab

[PATCH 4/5] target: Remove useless if statement

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

We do the same thing no matter which way the test goes, so just remove
the test and do what we're going to do.

The debug messages printed the wrong value of CMD_T_ACTIVE and don't
seem particularly useful, remove them too.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_tmr.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index c6e0293..d0b4dd9 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -331,18 +331,6 @@ static void core_tmr_drain_state_list(
 
 		fe_count = atomic_read(&cmd->t_fe_count);
 
-		if (!(cmd->transport_state & CMD_T_ACTIVE)) {
-			pr_debug("LUN_RESET: got CMD_T_ACTIVE for"
-				" cdb: %p, t_fe_count: %d dev: %p\n", cmd,
-				fe_count, dev);
-			cmd->transport_state |= CMD_T_ABORTED;
-			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-
-			core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
-			continue;
-		}
-		pr_debug("LUN_RESET: Got !CMD_T_ACTIVE for cdb: %p,"
-			" t_fe_count: %d dev: %p\n", cmd, fe_count, dev);
 		cmd->transport_state |= CMD_T_ABORTED;
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-- 
1.8.0

Re: [PATCH 4/5] target: Remove useless if statement

[Nicholas A. Bellinger]

On Wed, 2013-01-02 at 12:48 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> We do the same thing no matter which way the test goes, so just remove
> the test and do what we're going to do.
> 
> The debug messages printed the wrong value of CMD_T_ACTIVE and don't
> seem particularly useful, remove them too.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/target/target_core_tmr.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index c6e0293..d0b4dd9 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c
> @@ -331,18 +331,6 @@ static void core_tmr_drain_state_list(
>  
>  		fe_count = atomic_read(&cmd->t_fe_count);
>  
> -		if (!(cmd->transport_state & CMD_T_ACTIVE)) {
> -			pr_debug("LUN_RESET: got CMD_T_ACTIVE for"
> -				" cdb: %p, t_fe_count: %d dev: %p\n", cmd,
> -				fe_count, dev);
> -			cmd->transport_state |= CMD_T_ABORTED;
> -			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> -
> -			core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
> -			continue;
> -		}
> -		pr_debug("LUN_RESET: Got !CMD_T_ACTIVE for cdb: %p,"
> -			" t_fe_count: %d dev: %p\n", cmd, fe_count, dev);
>  		cmd->transport_state |= CMD_T_ABORTED;
>  		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  

Looks OK to drop.  Applying to for-next.

--nab

[PATCH 5/5] target: Remove never-used TMR_FABRIC_TMR enum value

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 include/target/target_core_base.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 7cae236..02ed017 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -210,7 +210,6 @@ enum tcm_tmreq_table {
 	TMR_LUN_RESET		= 5,
 	TMR_TARGET_WARM_RESET	= 6,
 	TMR_TARGET_COLD_RESET	= 7,
-	TMR_FABRIC_TMR		= 255,
 };
 
 /* fabric independent task management response values */
-- 
1.8.0

Re: [PATCH 5/5] target: Remove never-used TMR_FABRIC_TMR enum value

[Nicholas A. Bellinger]

On Wed, 2013-01-02 at 12:48 -0800, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  include/target/target_core_base.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 7cae236..02ed017 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -210,7 +210,6 @@ enum tcm_tmreq_table {
>  	TMR_LUN_RESET		= 5,
>  	TMR_TARGET_WARM_RESET	= 6,
>  	TMR_TARGET_COLD_RESET	= 7,
> -	TMR_FABRIC_TMR		= 255,
>  };
>  
>  /* fabric independent task management response values */

Applied to for-next.

Thanks again Roland!

--nab