Re: [PATCH 09/20] qla2xxx: Change check_stop_free to always return 1

[Quinn Tran]

On 12/8/15, 10:56 PM, "target-devel-ow...@vger.kernel.org on behalf of
Hannes Reinecke"  wrote:

>On 12/08/2015 01:48 AM, Himanshu Madhani wrote:
>> From: Quinn Tran 
>> 
>> change tcm_qla2xxx_check_stop_free to always return 1
>> to prevent transport_cmd_finish_abort from accidently
>> taking extra kref_put.
>> 
>> Signed-off-by: Quinn Tran 
>> Signed-off-by: Himanshu Madhani 
>> ---
>>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>>b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> index 74c6e9b..366142a 100644
>> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> @@ -314,7 +314,8 @@ static int tcm_qla2xxx_check_stop_free(struct
>>se_cmd *se_cmd)
>>  cmd->cmd_flags |= BIT_14;
>>  }
>>  
>> -return target_put_sess_cmd(se_cmd);
>> +target_put_sess_cmd(se_cmd);
>> +return 1;
>>  }
>>  
>>  /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release
>>underlying
>> 
>Odd. I would have expected target_put_sess_cmd() to never fail.

QT> It does not failed under normal usage.

>But if it does, doesn't it mean that the command is hosed, and we should
>have accessed it in the first place?
>Very suspicious.

QT> here are the cases I see.  Note: The patch series is in reference of
pre-Bart¹s fix.

- cmd_kref 2 -> 1, target_put_sess_cmd return true. This is Normal case.
Backend finished with the command and send it to the Front end.

- cmd_kref 1 -> 0, target_put_sess_cmd return true.
Case 1: Early free by TCM/TMR thread.  TMR thread call 2nd check_stop.

core_tmr_drain_tmr_list->transport_cmd_finish_abort->
transport_cmd_check_stop_to_fabric

- cmd_kref 0 -> -1, target_put_sess_cmd return false.   QLA does not call
³check_stop² in either cases below.
Case 1: QLA cause double free the command as the command is coming out of
HW queue. In other word QLA access stale pointer during cleanup/free.
Case 2: QLA driver beat TMR thread by a few nano second by freeing the
command 1st(cmd_kref=0).  TMR thread call the extra check_stop(cmd_kref =
-1).  transport_cmd_finish_abort do additional kref_put (cmd_kref = -2).

void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
{
if (transport_cmd_check_stop_to_fabric(cmd))
return;
if (remove)
transport_put_cmd(cmd);
}




>
>Cheers,
>
>Hannes
>-- 
>Dr. Hannes Reinecke  zSeries & Storage
>h...@suse.de +49 911 74053 688
>SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
>GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>--
>To unsubscribe from this list: send the line "unsubscribe target-devel" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

<>

Re: [PATCH 09/20] qla2xxx: Change check_stop_free to always return 1

[Hannes Reinecke]

On 12/08/2015 01:48 AM, Himanshu Madhani wrote:
> From: Quinn Tran 
> 
> change tcm_qla2xxx_check_stop_free to always return 1
> to prevent transport_cmd_finish_abort from accidently
> taking extra kref_put.
> 
> Signed-off-by: Quinn Tran 
> Signed-off-by: Himanshu Madhani 
> ---
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 74c6e9b..366142a 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -314,7 +314,8 @@ static int tcm_qla2xxx_check_stop_free(struct se_cmd 
> *se_cmd)
>   cmd->cmd_flags |= BIT_14;
>   }
>  
> - return target_put_sess_cmd(se_cmd);
> + target_put_sess_cmd(se_cmd);
> + return 1;
>  }
>  
>  /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release underlying
> 
Odd. I would have expected target_put_sess_cmd() to never fail.
But if it does, doesn't it mean that the command is hosed, and we should
have accessed it in the first place?
Very suspicious.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/20] qla2xxx: Change check_stop_free to always return 1

[Christoph Hellwig]

On Mon, Dec 07, 2015 at 07:48:56PM -0500, Himanshu Madhani wrote:
> From: Quinn Tran 
> 
> change tcm_qla2xxx_check_stop_free to always return 1
> to prevent transport_cmd_finish_abort from accidently
> taking extra kref_put.

This looks a bit fishy.  Even if this was the right behavior it's
something all something all drivers returning the value of
target_put_sess_cmd in their check_stop_free would need to do.

Can you explain the issue you're trying to address in a little more
detail?

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/20] qla2xxx: Change check_stop_free to always return 1

[Himanshu Madhani]

From: Quinn Tran 

change tcm_qla2xxx_check_stop_free to always return 1
to prevent transport_cmd_finish_abort from accidently
taking extra kref_put.

Signed-off-by: Quinn Tran 
Signed-off-by: Himanshu Madhani 
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 74c6e9b..366142a 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -314,7 +314,8 @@ static int tcm_qla2xxx_check_stop_free(struct se_cmd 
*se_cmd)
cmd->cmd_flags |= BIT_14;
}
 
-   return target_put_sess_cmd(se_cmd);
+   target_put_sess_cmd(se_cmd);
+   return 1;
 }
 
 /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release underlying
-- 
1.7.7

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html