linux-kernel - Re: [PATCH v3] ata: libata-eh: Honor all EH scheduling requests

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: <a01dc25c-16c0-2d72-23f1-d0955007fcc0@kernel.org>
Date:   Wed, 6 Sep 2023 15:49:27 +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 v3] ata: libata-eh: Honor all EH scheduling requests

On 9/6/23 15:33, linan666@...weicloud.com wrote:
> From: Li Nan <linan122@...wei.com>
> 
> 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 remains frozen and will never be enabled.
> 
> 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>

Looks good to me, but I think this needs a Fixes tag, and "Cc: stable".
Please add that.

> ---
> Changes in v3:
>  - change patch title, previously it was: "scsi: ata: Fix a race condition
>    between scsi error handler and ahci interrupt".
>  - drop the variable 'host' in ata_std_end_eh().
>  - improve commit message.
> 
>  drivers/ata/libata-eh.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 159ba6ba19eb..2d5ecd68b7e0 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);
>  
> @@ -946,9 +952,7 @@ EXPORT_SYMBOL_GPL(ata_std_sched_eh);
>   */
>  void ata_std_end_eh(struct ata_port *ap)
>  {
> -	struct Scsi_Host *host = ap->scsi_host;
> -
> -	host->host_eh_scheduled = 0;
> +	ap->scsi_host->host_eh_scheduled--;
>  }
>  EXPORT_SYMBOL(ata_std_end_eh);
>  
> @@ -3922,10 +3926,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