Re: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free.

[Hannes Reinecke <hare@suse.de>]

On 12/08/2015 01:48 AM, Himanshu Madhani wrote:
> From: Quinn Tran <quinn.tran@qlogic.com>
> 
> During LUN/Target reset, the TMR code attempt to intercept
> cmds and try to aborted them.  Current code assume cmds are
> always intercepted at the back end device.  The cleanup code
> would issue a "queue_status() & check_stop_free()" to terminate
> the command.  However, when a cmd is intercepted at the front
> end/Fabric layer, current code introduce premature free or
> cause Fabric to double free.
> 
> When command is intercepted at Fabric layer, it means a
> check_stop_free(cmd_kref--) has been called.  The extra
> check_stop_free in the Lun Reset cleanup code causes early
> free.  When a cmd in the Fabric layer is completed, the normal
> free code adds another another free which introduce a double free.
> 
> To fix the issue:
> - add a new flag/CMD_T_SENT_STATUS to track command that have
>  made it down to fabric layer after back end good/bad completion.
> - if cmd reach Fabric Layer at Lun Reset time, add an extra
>  cmd_kref count to prevent premature free.
> 
> Signed-off-by: Quinn Tran <quinn.tran@qlogic.com>
> Signed-off-by: Himanshu Madhani <himanshu.madhani@qlogic.com>
> ---
>  drivers/target/target_core_tmr.c       |   33 +++++++++++++++++++++++++++++++-
>  drivers/target/target_core_transport.c |   30 +++++++++++++++++++++++++++++
>  include/target/target_core_base.h      |    1 +
>  3 files changed, 63 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 28fb301..41f8b57 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c
> @@ -243,7 +243,9 @@ static void core_tmr_drain_state_list(
>  {
>  	LIST_HEAD(drain_task_list);
>  	struct se_cmd *cmd, *next;
> -	unsigned long flags;
> +	unsigned long flags, flags2;
> +	int rmkref;
> +	struct se_session *se_sess;
>  
>  	/*
>  	 * Complete outstanding commands with TASK_ABORTED SAM status.
> @@ -282,6 +284,16 @@ static void core_tmr_drain_state_list(
>  		if (prout_cmd == cmd)
>  			continue;
>  
> +		se_sess = cmd->se_sess;
> +		/* take an extra kref to prevent cmd free race condition. */
> +		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags2);
> +		if (!kref_get_unless_zero(&cmd->cmd_kref)) {
> +			/* cmd is already in the free process */
> +			spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags2);
> +			continue;
> +		}
> +		spin_unlock_irqrestore(&se_sess->sess_cmd_lock,	flags2);
> +
>  		list_move_tail(&cmd->state_list, &drain_task_list);
>  		cmd->state_active = false;
>  	}
> @@ -320,9 +332,28 @@ static void core_tmr_drain_state_list(
>  		target_stop_cmd(cmd, &flags);
>  
>  		cmd->transport_state |= CMD_T_ABORTED;
> +
> +		/* CMD_T_SENT_STATUS: cmd is down in fabric layer.
> +		 * A check stop has been called.  Keep the extra kref
> +		 * from above because core_tmr_handle_tas_abort will
> +		 * generate another check_stop.
> +		 *
> +		 * !CMD_T_SENT_STATUS: cmd intercepted at back end.
> +		 * Remove the extra kref from above because only
> +		 * 1 check_stop is required or generated by
> +		 * core_tmr_handle_tas_abort()
> +		 */
> +		rmkref = 0;
> +		if (!((cmd->t_state == TRANSPORT_COMPLETE) &&
> +		    (cmd->transport_state & CMD_T_SENT_STATUS)))
> +			rmkref = 1;
> +
>  		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  
>  		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
> +
> +		if (rmkref)
> +			target_put_sess_cmd(cmd);
>  	}
>  }
>  
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 4fdcee2..cdd18bf 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -639,9 +639,14 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
>  static void target_complete_failure_work(struct work_struct *work)
>  {
>  	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> +	unsigned long flags;
>  
>  	transport_generic_request_failure(cmd,
>  			TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE);
> +
> +	spin_lock_irqsave(&cmd->t_state_lock, flags);
> +	cmd->transport_state |= CMD_T_SENT_STATUS;
> +	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  }
>  
>  /*
> @@ -1659,6 +1664,7 @@ void transport_generic_request_failure(struct se_cmd *cmd,
>  		sense_reason_t sense_reason)
>  {
>  	int ret = 0, post_ret = 0;
> +	unsigned long flags;
>  
>  	pr_debug("-----[ Storage Engine Exception for cmd: %p ITT: 0x%08llx"
>  		" CDB: 0x%02x\n", cmd, cmd->tag, cmd->t_task_cdb[0]);
> @@ -1670,6 +1676,10 @@ void transport_generic_request_failure(struct se_cmd *cmd,
>  		(cmd->transport_state & CMD_T_STOP) != 0,
>  		(cmd->transport_state & CMD_T_SENT) != 0);
>  
> +	spin_lock_irqsave(&cmd->t_state_lock, flags);
> +	cmd->transport_state |= CMD_T_SENT_STATUS;
> +	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +
>  	/*
>  	 * For SAM Task Attribute emulation for failed struct se_cmd
>  	 */
> @@ -1951,6 +1961,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd)
>  static void transport_complete_qf(struct se_cmd *cmd)
>  {
>  	int ret = 0;
> +	unsigned long flags;
>  
>  	transport_complete_task_attr(cmd);
>  
> @@ -1986,6 +1997,10 @@ out:
>  	}
>  	transport_lun_remove_cmd(cmd);
>  	transport_cmd_check_stop_to_fabric(cmd);
> +
> +	spin_lock_irqsave(&cmd->t_state_lock, flags);
> +	cmd->transport_state |= CMD_T_SENT_STATUS;
> +	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  }
>  
>  static void transport_handle_queue_full(
> @@ -2032,6 +2047,7 @@ static void target_complete_ok_work(struct work_struct *work)
>  {
>  	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
>  	int ret;
> +	unsigned long flags;
>  
>  	/*
>  	 * Check if we need to move delayed/dormant tasks from cmds on the
> @@ -2060,6 +2076,10 @@ static void target_complete_ok_work(struct work_struct *work)
>  
>  		transport_lun_remove_cmd(cmd);
>  		transport_cmd_check_stop_to_fabric(cmd);
> +
> +		spin_lock_irqsave(&cmd->t_state_lock, flags);
> +		cmd->transport_state |= CMD_T_SENT_STATUS;
> +		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  		return;
>  	}
>  	/*
> @@ -2086,6 +2106,11 @@ static void target_complete_ok_work(struct work_struct *work)
>  
>  			transport_lun_remove_cmd(cmd);
>  			transport_cmd_check_stop_to_fabric(cmd);
> +
> +			spin_lock_irqsave(&cmd->t_state_lock, flags);
> +			cmd->transport_state |= CMD_T_SENT_STATUS;
> +			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +
>  			return;
>  		}
>  	}
> @@ -2136,6 +2161,7 @@ queue_rsp:
>  		ret = cmd->se_tfo->queue_status(cmd);
>  		if (ret == -EAGAIN || ret == -ENOMEM)
>  			goto queue_full;
> +
>  		break;
>  	default:
>  		break;
> @@ -2143,6 +2169,10 @@ queue_rsp:
>  
>  	transport_lun_remove_cmd(cmd);
>  	transport_cmd_check_stop_to_fabric(cmd);
> +
> +	spin_lock_irqsave(&cmd->t_state_lock, flags);
> +	cmd->transport_state |= CMD_T_SENT_STATUS;
> +	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  	return;
>  
>  queue_full:
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index aabf0ac..efccd71 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -490,6 +490,7 @@ struct se_cmd {
>  #define CMD_T_DEV_ACTIVE	(1 << 7)
>  #define CMD_T_REQUEST_STOP	(1 << 8)
>  #define CMD_T_BUSY		(1 << 9)
> +#define CMD_T_SENT_STATUS (1 << 10)
>  	spinlock_t		t_state_lock;
>  	struct kref		cmd_kref;
>  	struct completion	t_transport_stop_comp;
> 
Same here: using bitops would save you taking the spinlock when
modifying flags.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html