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

Message-ID: <8a34d182-0adb-fcf5-6c3f-0ffa8392f615@huaweicloud.com>
Date:   Tue, 5 Sep 2023 21:09:06 +0800
From:   Li Nan <linan666@...weicloud.com>
To:     Damien Le Moal <dlemoal@...nel.org>
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



在 2023/9/5 15:44, Damien Le Moal 写道:
> 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().
> 

Thanks for your review. I will improve the commmit message.

>>
>> 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.
> 

I agree. I will change it in next version.

>>   }
>>   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;
>>   }
>>   
>>   /**
> 

-- 
Thanks,
Nan