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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f33816ef-ce26-1501-99b6-c75f91c6d8f5@huaweicloud.com>
Date:   Tue, 22 Aug 2023 17:20:33 +0800
From:   Li Nan <linan666@...weicloud.com>
To:     Niklas Cassel <Niklas.Cassel@....com>,
        "linan666@...weicloud.com" <linan666@...weicloud.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

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

   ata_do_eh+0x70/0xf0
   ata_sff_error_handler+0xde/0x170
   ata_bmdma_error_handler+0x5b/0x1e0
   ata_scsi_port_error_handler+0x5b1/0xae0
   ata_scsi_error+0xb9/0x110
   scsi_error_handler+0x137/0x8d0

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 35e03679b0bf..82f032934ae1 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -962,7 +962,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);
> 
> 
> 
> ...and see if that improves things for you.
> 
> 
> 
> Kind regards,
> Niklas.

-- 
Thanks,
Nan