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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 21 Aug 2023 13:51:18 +0000
From:   Niklas Cassel <Niklas.Cassel@....com>
To:     "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>,
        "linan122@...wei.com" <linan122@...wei.com>,
        "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

On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@...weicloud.com wrote:
> From: Li Nan <linan122@...wei.com>
> 
> interrupt                            scsi_eh
> 
> ahci_error_intr
>   =>ata_port_freeze
>     =>__ata_port_freeze
>       =>ahci_freeze (turn IRQ off)
>     =>ata_port_abort
>       =>ata_port_schedule_eh
>         =>shost->host_eh_scheduled++;
>         host_eh_scheduled = 1
>                                      scsi_error_handler
>                                        =>ata_scsi_error
>                                          =>ata_scsi_port_error_handler
>                                            =>ahci_error_handler
>                                            . =>sata_pmp_error_handler
>                                            .   =>ata_eh_thaw_port
>                                            .     =>ahci_thaw (turn IRQ on)
> ahci_error_intr                            .
>   =>ata_port_freeze                        .
>     =>__ata_port_freeze                    .
>       =>ahci_freeze (turn IRQ off)         .
>     =>ata_port_abort                       .
>       =>ata_port_schedule_eh               .
>         =>shost->host_eh_scheduled++;      .
>         host_eh_scheduled = 2              .
>                                            =>ata_std_end_eh
>                                              =>host->host_eh_scheduled = 0;

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

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:

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