linux-kernel - Re: [PATCH v2] 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: <c4d397c4-1969-935f-6dc3-cf7775d96f5c@kernel.org>
Date:   Tue, 5 Sep 2023 16:44:23 +0900
From:   Damien Le Moal <dlemoal@...nel.org>
To:     linan666@...weicloud.com
Cc:     linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
        linan122@...wei.com, yukuai3@...wei.com, yi.zhang@...wei.com,
        houtao1@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH v2] scsi: ata: Fix a race condition between scsi error
 handler and ahci interrupt

On 9/5/23 12:48, linan666@...weicloud.com wrote:
> From: Li Nan <linan122@...wei.com>

Please change the patch title to something like:

ata: libata-eh: Honor all EH scheduling requests

Because the issue could happen I think with other ata drivers than AHCI.


> 
> If a disk is removed and quickly inserted when an I/O error is processing,
> the disk may not be able to be re-added. The function call timeline is as
> follows:
> 
>   interrupt                            scsi_eh
> 
>   ahci_error_intr
>    ata_port_freeze
>     __ata_port_freeze
>      =>ahci_freeze (turn IRQ off)
>     ata_port_abort
>      ata_do_link_abort
>       ata_port_schedule_eh
>        =>ata_std_sched_eh
>         ata_eh_set_pending
> 	 set EH_PENDING
>         scsi_schedule_eh
>          shost->host_eh_scheduled++ (=1)
>                                        scsi_error_handler
>                                         =>ata_scsi_error
>                                          ata_scsi_port_error_handler
> 					  clear EH_PENDING
>                                           =>ahci_error_handler
>                                           . sata_pmp_error_handler
>                                           .  ata_eh_reset
>                                           .   ata_eh_thaw_port
>                                           .   . =>ahci_thaw (turn IRQ on)
>   ahci_error_intr			  .   .
>    ata_port_freeze			  .   .
>     __ata_port_freeze			  .   .
>      =>ahci_freeze (turn IRQ off)	  .   .
>     ...					  .   .
>         ata_eh_set_pending		  .   .
> 	 set EH_PENDING			  .   .
>         scsi_schedule_eh		  .   .
>          shost->host_eh_scheduled++ (=2)  .   .
> 					  .   clear EH_PENDING
> 					  check EH_PENDING
>                                           =>ata_std_end_eh
>                                            host->host_eh_scheduled = 0;
> 
> 'host_eh_scheduled' is 0 and scsi eh thread will not be scheduled again.
> The ata port remain freeze and will never be enabled.

s/remain freeze/remains frozen

> 
> Decrease ’host_eh_scheduled‘ instead of set it to 0 roughly and move
> WARN_ON of nr_active_links to ata_scsi_port_error_handler().

To fix this issue, decrease ’host_eh_scheduled‘ instead of setting it to 0 so
that EH is scheduled again to re-enable the port. Also move the update of
nr_active_links to 0 when host_eh_scheduled is 0 to ata_scsi_port_error_handler().

> 
> Reported-by: luojian <luojian5@...wei.com>
> Signed-off-by: Li Nan <linan122@...wei.com>
> ---
> Changes in v2:
>   - fix the bug by decrease 'host_eh_scheduled’
>   - improve commit message.
> 
>  drivers/ata/libata-eh.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 159ba6ba19eb..b9454756ecde 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -735,6 +735,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);
>  
> @@ -948,7 +954,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--;

While at it, please drop the host variable:

	ap->scsi_host->host_eh_scheduled--;

is just fine.

>  }
>  EXPORT_SYMBOL(ata_std_end_eh);
>  
> @@ -3922,10 +3928,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;
>  }
>  
>  /**

-- 
Damien Le Moal
Western Digital Research