linux-kernel - Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <033cb727-35b5-3845-64b1-e698891d70b1@huaweicloud.com>
Date:   Mon, 4 Sep 2023 21:00:07 +0800
From:   Li Nan <linan666@...weicloud.com>
To:     Niklas Cassel <Niklas.Cassel@....com>
Cc:     "dlemoal@...nel.org" <dlemoal@...nel.org>,
        "linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "yukuai3@...wei.com" <yukuai3@...wei.com>,
        "yi.zhang@...wei.com" <yi.zhang@...wei.com>,
        "houtao1@...wei.com" <houtao1@...wei.com>,
        "yangerkun@...wei.com" <yangerkun@...wei.com>
Subject: Re: [PATCH] scsi: ata: Fix a race condition between scsi error
 handler and ahci interrupt



在 2023/9/4 19:57, Niklas Cassel 写道:
> On Mon, Sep 04, 2023 at 07:45:51PM +0800, Li Nan wrote:
>>
>>
>> 在 2023/8/22 18:30, Niklas Cassel 写道:
>>> On Tue, Aug 22, 2023 at 05:20:33PM +0800, Li Nan wrote:
>>>> Thanks for your reply, Niklas.
>>>>
>>>> 在 2023/8/21 21:51, Niklas Cassel 写道:
>>>>> On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@...weicloud.com wrote:
>>>>
>>>> [snip]
>>>>
>>>>>
>>>>> Hello Li Nan,
>>>>>
>>>>> I do not understand why the code in:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731
>>>>>
>>>>> does not kick in, and repeats EH.
>>>>>
>>>>>
>>>>> EH_PENDING is cleared before ->error_handler() is called:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697
>>>>>
>>>>> So ahci_error_intr() from the second error interrupt, which is called after
>>>>> thawing the port, should have called ata_std_sched_eh(), which calls
>>>>> ata_eh_set_pending(), which should have set EH_PENDING:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884
>>>>>
>>>>>
>>>>>
>>>>> My only guess is that after thawing the port:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807
>>>>>
>>>>> The second error irq comes, and sets EH_PENDING,
>>>>> but then this silly code might clear it:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837
>>>>>
>>>>
>>>> Yeah, I think so.
>>>>
>>>>> I think the best way would be if we could improve this "spurious error
>>>>> condition check"... because if this is indeed the code that clears EH_PENDING
>>>>> for you, then this code basically makes the "goto repeat" code in
>>>>> ata_scsi_port_error_handler() useless...
>>>>>
>>>>>
>>>>> An alternative to improving the "spurious error condition check" might be for
>>>>> you to try something like:
>>>>>
>>>>
>>>> We have used this solution before, but it will case WARN_ON in
>>>> ata_eh_finish() as below:
>>>>
>>>>     WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016
>>>> ata_eh_finish+0x15a/0x170
>>>
>>> Ok.
>>>
>>> How about if you simply move the WARN_ON to ata_scsi_port_error_handler()
>>> as well:
>>>
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>> index 35e03679b0bf..5be2fc651131 100644
>>> --- a/drivers/ata/libata-eh.c
>>> +++ b/drivers/ata/libata-eh.c
>>> @@ -741,6 +741,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>>>                    */
>>>                   ap->ops->end_eh(ap);
>>> +               if (!ap->scsi_host->host_eh_scheduled) {
>>> +                       /* make sure nr_active_links is zero after EH */
>>> +                       WARN_ON(ap->nr_active_links);
>>> +                       ap->nr_active_links = 0;
>>> +               }
>>> +
>>>                   spin_unlock_irqrestore(ap->lock, flags);
>>>                   ata_eh_release(ap);
>>>           } else {
>>> @@ -962,7 +968,7 @@ void ata_std_end_eh(struct ata_port *ap)
>>>    {
>>>           struct Scsi_Host *host = ap->scsi_host;
>>> -       host->host_eh_scheduled = 0;
>>> +       host->host_eh_scheduled--;
>>>    }
>>>    EXPORT_SYMBOL(ata_std_end_eh);
>>> @@ -3948,10 +3954,6 @@ void ata_eh_finish(struct ata_port *ap)
>>>                           }
>>>                   }
>>>           }
>>> -
>>> -       /* make sure nr_active_links is zero after EH */
>>> -       WARN_ON(ap->nr_active_links);
>>> -       ap->nr_active_links = 0;
>>>    }
>>>    /**
>>>
>>>
>>>
>>> Kind regards,
>>> Niklas
>>
>> We have tested this patch and it can fix the bug. Thank you so much. :)
> 
> Awesome! :)
> 
> Please send out a real patch, so that it is easier for the maintainer to
> apply.
> 
> No need to give any credit to me.
> 
> 
> Kind regards,
> Niklas

It is my pleasure. I will send v2 later.

-- 
Thanks,
Nan