On 12/8/15, 10:56 PM, "target-devel-owner@xxxxxxxxxxxxxxx on behalf of
Hannes Reinecke" <target-devel-owner@xxxxxxxxxxxxxxx on behalf of
hare@xxxxxxx> wrote:
>On 12/08/2015 01:48 AM, Himanshu Madhani wrote:
>> From: Quinn Tran <quinn.tran@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
>> ---
>> 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@xxxxxxx +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@xxxxxxxxxxxxxxx
>More majordomo info at http://vger.kernel.org/majordomo-info.html
<<attachment: winmail.dat>>
