Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK

From	Bart Van Assche <Bart.VanAssche@sandisk.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK
Date	2017-06-05 18:00 +0200
Message-ID	<tP2A9-4My-1@gated-at.bofh.it> (permalink)
References	<tOpp7-4fW-3@gated-at.bofh.it> <tOpp7-4fW-7@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Sat, 2017-06-03 at 22:10 +0000, Nicholas A. Bellinger wrote:
> +static bool target_lookup_lun_from_tag(struct se_session *se_sess, u64 tag,
> +				       u64 *unpacked_lun)
> +{
> +	struct se_cmd *se_cmd;
> +	unsigned long flags;
> +	bool ret = false;
> +
> +	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> +	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list) {
> +		if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> +			continue;
> +
> +		if (se_cmd->tag == tag) {
> +			*unpacked_lun = se_cmd->orig_fe_lun;
> +			ret = true;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +
> +	return ret;
> +}
> +
>  /**
>   * target_submit_tmr - lookup unpacked lun and submit uninitialized se_cmd
>   *                     for TMR CDBs
> @@ -1639,19 +1662,31 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
>  		core_tmr_release_req(se_cmd->se_tmr_req);
>  		return ret;
>  	}
> +	/*
> +	 * If this is ABORT_TASK with no explicit fabric provided LUN,
> +	 * go ahead and search active session tags for a match to figure
> +	 * out unpacked_lun for the original se_cmd.
> +	 */
> +	if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
> +		if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
> +			goto failure;
> +	}
>  
>  	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> -	if (ret) {
> -		/*
> -		 * For callback during failure handling, push this work off
> -		 * to process context with TMR_LUN_DOES_NOT_EXIST status.
> -		 */
> -		INIT_WORK(&se_cmd->work, target_complete_tmr_failure);
> -		schedule_work(&se_cmd->work);
> -		return 0;
> -	}
> +	if (ret)
> +		goto failure;
> +
>  	transport_generic_handle_tmr(se_cmd);
>  	return 0;

Hello Nic,

So after LUN lookup has finished sess_cmd_lock lock is dropped, a context
switch occurs due to the queue_work() call in transport_generic_handle_tmr()
and next core_tmr_abort_task() reacquires that lock? Sorry but I'm afraid
that if after that lock is dropped and before it is reacquired a command
finishes and the initiator reuses the same tag for another command for the
same LUN that this may result in the wrong command being aborted.

Bart.

Thread

[PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-04 00:10 +0200
  [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT_TASK "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-04 00:10 +0200
    Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support  for ABORT_TASK Bart Van Assche <Bart.VanAssche@sandisk.com> - 2017-06-05 18:00 +0200
    Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support  for ABORT_TASK "Tran, Quinn" <Quinn.Tran@cavium.com> - 2017-06-07 03:20 +0200
  [PATCH 1/3] target: Add support for TMR percpu reference counting "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-04 00:10 +0200
    Re: [PATCH 1/3] target: Add support for TMR percpu reference counting "Tran, Quinn" <Quinn.Tran@cavium.com> - 2017-06-07 03:20 +0200
  Re: [PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support "Madhani, Himanshu" <Himanshu.Madhani@cavium.com> - 2017-06-05 17:40 +0200
  Re: [PATCH 0/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support "Madhani, Himanshu" <Himanshu.Madhani@cavium.com> - 2017-06-07 07:10 +0200

csiph-web