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

[Quinn Tran <quinn.tran@qlogic.com>]

On 12/8/15, 10:56 PM, "target-devel-owner@vger.kernel.org on behalf of
Hannes Reinecke" <target-devel-owner@vger.kernel.org on behalf of
hare@suse.de> wrote:

>On 12/08/2015 01:48 AM, Himanshu Madhani wrote:
>> From: Quinn Tran <quinn.tran@qlogic.com>
>> 
>> 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 <quinn.tran@qlogic.com>
>> Signed-off-by: Himanshu Madhani <himanshu.madhani@qlogic.com>
>> ---
>>  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
>hare@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 majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html