[PATCH] powerpc/eeh: Fix recursive locking on devices without EEH sensitive driver

[PATCH] powerpc/eeh: Fix recursive locking on devices without EEH sensitive driver

[Shivaprasad G Bhat]

The commit 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug
safe") refactored the EEH code such that the pci_rescan_remove_lock is
held at the beginning of eeh_handle_normal_event() and the
eeh_reset_device() is called with that lock being held. Looks like the
commit missed to remove the existing lock/unlock inside eeh_rmv_device()
which is no longer necessary. This is causing the eehd to hang on the
lock which it actually holds when that code path is taken.

[<0>] 0xc00000011c78f870
[<0>] __switch_to+0xfc/0x1a0
[<0>] pci_lock_rescan_remove+0x30/0x44
[<0>] eeh_rmv_device+0x290/0x2e0
[<0>] eeh_pe_dev_traverse+0x80/0x130
[<0>] eeh_reset_device+0xcc/0x23c
[<0>] eeh_handle_normal_event+0x830/0xa80
[<0>] eeh_event_handler+0xf8/0x190
[<0>] kthread+0x194/0x1b0
[<0>] start_kernel_thread+0x14/0x18

The issue is seen for cases where the errors are detected on the PHB
directly AND|OR for devices where the driver error_detected() returns
PCI_ERS_RESULT_NEED_RESET, and driver being not EEH sensitive(i.e no
error handlers like slot_reset(), resume() etc defined).

Fixes: 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug safe")
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 arch/powerpc/kernel/eeh_driver.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 028f69158532..d64cce17a4e0 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -533,9 +533,7 @@ static void eeh_rmv_device(struct eeh_dev *edev, void *userdata)
 		if (rmv_data)
 			list_add(&edev->rmv_entry, &rmv_data->removed_vf_list);
 	} else {
-		pci_lock_rescan_remove();
 		pci_stop_and_remove_bus_device(dev);
-		pci_unlock_rescan_remove();
 	}
 }
 

Re: [PATCH] powerpc/eeh: Fix recursive locking on devices without EEH sensitive driver

[Ritesh Harjani]

Shivaprasad G Bhat <sbhat@linux.ibm.com> writes:

hey, sorry looks like this fall through the cracks.

> The commit 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug
> safe") refactored the EEH code such that the pci_rescan_remove_lock is
> held at the beginning of eeh_handle_normal_event() and the
> eeh_reset_device() is called with that lock being held. Looks like the
> commit missed to remove the existing lock/unlock inside eeh_rmv_device()
> which is no longer necessary. This is causing the eehd to hang on the
> lock which it actually holds when that code path is taken.
>
> [<0>] 0xc00000011c78f870
> [<0>] __switch_to+0xfc/0x1a0
> [<0>] pci_lock_rescan_remove+0x30/0x44
> [<0>] eeh_rmv_device+0x290/0x2e0
> [<0>] eeh_pe_dev_traverse+0x80/0x130
> [<0>] eeh_reset_device+0xcc/0x23c
> [<0>] eeh_handle_normal_event+0x830/0xa80
> [<0>] eeh_event_handler+0xf8/0x190
> [<0>] kthread+0x194/0x1b0
> [<0>] start_kernel_thread+0x14/0x18
>

yup. I see eeh_handle_normal_event(), already holds this lock,
pci_lock_rescan_remove().

And eeh_rmv_device() only ever gets called from

eeh_handle_normal_event()
  eeh_reset_device()
    eeh_pe_dev_traverse(pe, eeh_rmv_device,...)

OR

eeh_handle_normal_event()),
    eeh_pe_dev_traverse(pe, eeh_rmv_device,...)

In both the paths, eeh_handle_normal_event() holds this lock. Although I
am not an expert in eeh area - but looking at the relevant code paths,
the race looks real and this patch fixes that. So feel free to add: 

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


I wonder whether clang context analyzer [1] could catch this. This is a
good candidate to try that out.

[1]: https://lore.kernel.org/lkml/20251219154418.3592607-1-elver@google.com/


> The issue is seen for cases where the errors are detected on the PHB
> directly AND|OR for devices where the driver error_detected() returns
> PCI_ERS_RESULT_NEED_RESET, and driver being not EEH sensitive(i.e no
> error handlers like slot_reset(), resume() etc defined).
>

I am just wondering how did we catch this issue and whether we have some
ways to test eeh scenarios. Although the patch looks good to me.


> Fixes: 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug safe")

Cc: stable@vger.kernel.org 

Let's add the above tag so that this also goes to stable trees.  Since
it's been sometime since this was last posted - maybe we should rebase
and resend this with the above tag.

-ritesh

> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>  arch/powerpc/kernel/eeh_driver.c |    2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 028f69158532..d64cce17a4e0 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -533,9 +533,7 @@ static void eeh_rmv_device(struct eeh_dev *edev, void *userdata)
>  		if (rmv_data)
>  			list_add(&edev->rmv_entry, &rmv_data->removed_vf_list);
>  	} else {
> -		pci_lock_rescan_remove();
>  		pci_stop_and_remove_bus_device(dev);
> -		pci_unlock_rescan_remove();
>  	}
>  }
>  

Re: [PATCH] powerpc/eeh: Fix recursive locking on devices without EEH sensitive driver

[Shivaprasad G Bhat]

Hi Ritesh,

Thanks for the review.

On 7/8/26 8:36 PM, Ritesh Harjani (IBM) wrote:
> Shivaprasad G Bhat <sbhat@linux.ibm.com> writes:
>
> hey, sorry looks like this fall through the cracks.
>
>> The commit 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug
>> safe") refactored the EEH code such that the pci_rescan_remove_lock is
>> held at the beginning of eeh_handle_normal_event() and the
>> eeh_reset_device() is called with that lock being held. Looks like the
>> commit missed to remove the existing lock/unlock inside eeh_rmv_device()
>> which is no longer necessary. This is causing the eehd to hang on the
>> lock which it actually holds when that code path is taken.
>>
>> [<0>] 0xc00000011c78f870
>> [<0>] __switch_to+0xfc/0x1a0
>> [<0>] pci_lock_rescan_remove+0x30/0x44
>> [<0>] eeh_rmv_device+0x290/0x2e0
>> [<0>] eeh_pe_dev_traverse+0x80/0x130
>> [<0>] eeh_reset_device+0xcc/0x23c
>> [<0>] eeh_handle_normal_event+0x830/0xa80
>> [<0>] eeh_event_handler+0xf8/0x190
>> [<0>] kthread+0x194/0x1b0
>> [<0>] start_kernel_thread+0x14/0x18
>>
> yup. I see eeh_handle_normal_event(), already holds this lock,
> pci_lock_rescan_remove().
>
> And eeh_rmv_device() only ever gets called from
>
> eeh_handle_normal_event()
>    eeh_reset_device()
>      eeh_pe_dev_traverse(pe, eeh_rmv_device,...)
>
> OR
>
> eeh_handle_normal_event()),
>      eeh_pe_dev_traverse(pe, eeh_rmv_device,...)
>
> In both the paths, eeh_handle_normal_event() holds this lock. Although I
> am not an expert in eeh area - but looking at the relevant code paths,
> the race looks real and this patch fixes that. So feel free to add:
>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>
> I wonder whether clang context analyzer [1] could catch this. This is a
> good candidate to try that out.
>
> [1]: https://lore.kernel.org/lkml/20251219154418.3592607-1-elver@google.com/


I did give it a try.

https://gist.github.com/shivaprasadbhat/473ea8bcb63afda669bcb5de24ac22f8

It couldnt catch it. May need some more work.


>> The issue is seen for cases where the errors are detected on the PHB
>> directly AND|OR for devices where the driver error_detected() returns
>> PCI_ERS_RESULT_NEED_RESET, and driver being not EEH sensitive(i.e no
>> error handlers like slot_reset(), resume() etc defined).
>>
> I am just wondering how did we catch this issue and whether we have some
> ways to test eeh scenarios. Although the patch looks good to me.


Two ways to recreate, Mostly with drivers not having eeh_handlers like

slot_reset(), resume defined.

- On Baremetal, eeh on multi-function device bound to vfio.

- On PowerVM, eeh on a device behind a switch or IO drawer.


>> Fixes: 1010b4c012b0 ("powerpc/eeh: Make EEH driver device hotplug safe")
> Cc: stable@vger.kernel.org
>
> Let's add the above tag so that this also goes to stable trees.  Since
> it's been sometime since this was last posted - maybe we should rebase
> and resend this with the above tag.

Sure.


Thanks,

Shivaprasad

> -ritesh
>
>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/eeh_driver.c |    2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
>> index 028f69158532..d64cce17a4e0 100644
>> --- a/arch/powerpc/kernel/eeh_driver.c
>> +++ b/arch/powerpc/kernel/eeh_driver.c
>> @@ -533,9 +533,7 @@ static void eeh_rmv_device(struct eeh_dev *edev, void *userdata)
>>   		if (rmv_data)
>>   			list_add(&edev->rmv_entry, &rmv_data->removed_vf_list);
>>   	} else {
>> -		pci_lock_rescan_remove();
>>   		pci_stop_and_remove_bus_device(dev);
>> -		pci_unlock_rescan_remove();
>>   	}
>>   }
>>