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.
