linux-kernel - Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support for ABORT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 08 Jun 2017 22:52:24 -0700
From:   "Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:     Bart Van Assche <Bart.VanAssche@...disk.com>
Cc:     "target-devel@...r.kernel.org" <target-devel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mchristi@...hat.com" <mchristi@...hat.com>,
        "himanshu.madhani@...ium.com" <himanshu.madhani@...ium.com>,
        "quinn.tran@...ium.com" <quinn.tran@...ium.com>,
        "hare@...e.com" <hare@...e.com>, "hch@....de" <hch@....de>
Subject: Re: [PATCH 2/3] target: Add TARGET_SCF_LOOKUP_LUN_FROM_TAG support
 for ABORT_TASK

On Mon, 2017-06-05 at 15:57 +0000, Bart Van Assche wrote:
> 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.

This is only getting a unpacked_lun to determine where the incoming
ABORT_TASK should perform a se_lun lookup and percpu ref upon.

The scenario you are talking about would require a tag be reused on the
same session for the same device between when the ABORT_TASK is
dispatched here, and actually processed.

Because qla2xxx doesn't reuse tags, it's not a problem since it's the
only consumer of TARGET_SCF_LOOKUP_LUN_FROM_TAG.

Since Himanshu and Quinn are OK it with, I'm OK with it.

If you have another driver that needs to use this logic where an
ABORT_TASK doesn't know the unpacked_lun to reference, and does reuse
tags then I'd consider a patch for that.