Re: [PATCH 1/2] iscsi-target: Fix TMR reference leak during session shutdown

[Nicholas A. Bellinger]

On Thu, 2017-03-30 at 17:08 +0000, Bart Van Assche wrote:
> On Thu, 2017-03-30 at 08:29 +0000, Nicholas A. Bellinger wrote:
> > diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> > index 5041a9c..b464033 100644
> > --- a/drivers/target/iscsi/iscsi_target_util.c
> > +++ b/drivers/target/iscsi/iscsi_target_util.c
> > @@ -737,21 +737,23 @@ void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
> >  {
> >  	struct se_cmd *se_cmd = NULL;
> >  	int rc;
> > +	bool op_scsi = false;
> >  	/*
> >  	 * Determine if a struct se_cmd is associated with
> >  	 * this struct iscsi_cmd.
> >  	 */
> >  	switch (cmd->iscsi_opcode) {
> >  	case ISCSI_OP_SCSI_CMD:
> > -		se_cmd = &cmd->se_cmd;
> > -		__iscsit_free_cmd(cmd, true, shutdown);
> > +		op_scsi = true;
> >  		/*
> >  		 * Fallthrough
> >  		 */
> >  	case ISCSI_OP_SCSI_TMFUNC:
> > -		rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown);
> > -		if (!rc && shutdown && se_cmd && se_cmd->se_sess) {
> > -			__iscsit_free_cmd(cmd, true, shutdown);
> > +		se_cmd = &cmd->se_cmd;
> > +		__iscsit_free_cmd(cmd, op_scsi, shutdown);
> > +		rc = transport_generic_free_cmd(se_cmd, shutdown);
> > +		if (!rc && shutdown && se_cmd->se_sess) {
> > +			__iscsit_free_cmd(cmd, op_scsi, shutdown);
> >  			target_put_sess_cmd(se_cmd);
> >  		}
> >  		break;
> 
> Hello Nic,
> 
> I agree that this patch fixes a leak. However, an existing bug in
> iscsit_free_cmd() is not addressed by this patch. Before the TMF code started
> using kref_get() / kref_put() it was possible for transport_generic_free_cmd()
> to determine whether or not iscsit_free_cmd() should call __iscsit_free_cmd()
> by checking the command reference count. I think that since the TMF code
> manipulates the command reference count it is no longer possible for
> transport_generic_free_cmd() to determine this. If iscsit_free_cmd() is called
> while a LUN RESET is in progress then the return value of
> transport_generic_free_cmd() will be wrong. 

No.  Your assumption is incorrect wrt transport_generic_free_cmd()
having a wrong return value during LUN_RESET.

It's incorrect because when iscsit_free_cmd() is called with
shutdown=true resulting in transport_generic_free_cmd() with
wait_for_tasks=true, target_wait_free_cmd() checks for CMD_T_ABORTED to
determine if se_cmd has been aborted by target_core_tmr.c logic, and
returns aborted=true back up to transport_generic_free_cmd().

When transport_generic_free_cmd() gets aborted=true, it waits for
se_cmd->cmd_wait_comp to finish and calls cmd->se_tfo->release_cmd()
to release se_cmd back to the pre-allocated session pool.

At this point, transport_generic_free_cmd() with aborted=true must
return '1' it's caller, signaling iscsit_free_cmd() to not attempt to
perform the extra __iscsi_free_cmd() or target_put_sess_cmd(), because
se_cmd has already been released back to the session pool.