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

Quinn Tran
2015-12-10T01:06:53Z

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