All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hannes Reinecke <hare@suse.de>
To: Himanshu Madhani <himanshu.madhani@qlogic.com>,
	target-devel@vger.kernel.org, nab@linux-iscsi.org
Cc: giridhar.malavali@qlogic.com, linux-scsi@vger.kernel.org
Subject: Re: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free.
Date: Wed, 9 Dec 2015 08:03:13 +0100	[thread overview]
Message-ID: <5667D231.9080702@suse.de> (raw)
In-Reply-To: <1449535747-2850-13-git-send-email-himanshu.madhani@qlogic.com>

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

  parent reply	other threads:[~2015-12-09  7:03 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-08  0:48 [PATCH 00/20] qla2xxx: Patches for target-pending branch Himanshu Madhani
2015-12-08  0:48 ` [PATCH 01/20] qla2xxx: Enable Extended Login support Himanshu Madhani
2015-12-08 15:51   ` Hannes Reinecke
2015-12-08 19:35     ` Himanshu Madhani
2015-12-08  0:48 ` [PATCH 02/20] qla2xxx: Enable Exchange offload support Himanshu Madhani
2015-12-08 15:52   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 03/20] qla2xxx: Enable Target counters in DebugFS Himanshu Madhani
2015-12-08 15:52   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 04/20] qla2xxx: Add FW resource count " Himanshu Madhani
2015-12-08 15:53   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 05/20] qla2xxx: Added interface to send ELS commands from driver Himanshu Madhani
2015-12-08  2:10   ` kbuild test robot
2015-12-08  2:10   ` [PATCH] qla2xxx: fix ifnullfree.cocci warnings kbuild test robot
2015-12-08 15:54   ` [PATCH 05/20] qla2xxx: Added interface to send ELS commands from driver Hannes Reinecke
2015-12-08  0:48 ` [PATCH 06/20] qla2xxx: Delete session if initiator is gone from FW Himanshu Madhani
2015-12-08  1:41   ` kbuild test robot
2015-12-08 15:58   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 07/20] qla2xxx: Wait for all conflicts before ack'ing PLOGI Himanshu Madhani
2015-12-08 16:00   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 08/20] qla2xxx: Replace QLA_TGT_STATE_ABORTED with a bit Himanshu Madhani
2015-12-08 16:01   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 09/20] qla2xxx: Change check_stop_free to always return 1 Himanshu Madhani
2015-12-08  2:33   ` Christoph Hellwig
2015-12-09  6:56   ` Hannes Reinecke
2015-12-10  1:06     ` Quinn Tran
2015-12-08  0:48 ` [PATCH 10/20] qla2xxx: Fix interaction issue between qla2xxx and Target Core Module Himanshu Madhani
2015-12-08  2:37   ` Christoph Hellwig
2015-12-09 22:07     ` Quinn Tran
2015-12-14 10:34       ` Christoph Hellwig
2015-12-14 21:59         ` Quinn Tran
2015-12-09  7:01   ` Hannes Reinecke
2015-12-09 22:41     ` Quinn Tran
2015-12-08  0:48 ` [PATCH 11/20] qla2xxx: Add TAS detection for kernel 3.15 n newer Himanshu Madhani
2015-12-08  2:48   ` Christoph Hellwig
2015-12-09 20:24     ` Quinn Tran
2015-12-14 10:37       ` Christoph Hellwig
2015-12-14 22:00         ` Quinn Tran
2015-12-09  7:02   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 12/20] target/tmr: LUN reset cause cmd premature free Himanshu Madhani
2015-12-08  2:48   ` Christoph Hellwig
2015-12-09 20:11     ` Quinn Tran
2016-01-04  7:44     ` Bart Van Assche
2015-12-09  7:03   ` Hannes Reinecke [this message]
2015-12-08  0:49 ` [PATCH 13/20] qla2xxx: Remove dependency on hardware_lock to reduce lock contention Himanshu Madhani
2015-12-08  0:49 ` [PATCH 14/20] qla2xxx: Add irq affinity notification Himanshu Madhani
2015-12-08  0:49 ` [PATCH 15/20] qla2xxx: Add selective command queuing Himanshu Madhani
2015-12-08  0:49 ` [PATCH 16/20] qla2xxx: Move atioq to a different lock to reduce lock contention Himanshu Madhani
2015-12-08  0:49 ` [PATCH 17/20] qla2xxx: Disable ZIO at start time Himanshu Madhani
2015-12-08  0:49 ` [PATCH 18/20] qla2xxx: Set all queues to 4k Himanshu Madhani
2015-12-08  0:49 ` [PATCH 19/20] qla2xxx: Add bulk send for atio & ctio completion paths Himanshu Madhani
2015-12-08  0:49 ` [PATCH 20/20] qla2xxx: Check for online flag instead of active reset when transmitting responses Himanshu Madhani

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5667D231.9080702@suse.de \
    --to=hare@suse.de \
    --cc=giridhar.malavali@qlogic.com \
    --cc=himanshu.madhani@qlogic.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=nab@linux-iscsi.org \
    --cc=target-devel@vger.kernel.org \
    --subject='Re: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.