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

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

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. I will post a few patches that not
only address what I just described but also the leak fixed by this patch.

Bart.