Re: [PATCH v17 05/11] cxl: Limit CXL-CPER kfifo registration functions scope

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deadlock risk in CPER kfifo registration functions due to non-IRQ-safe spinlocks.
--

commit 49bcf423cba32ca4c5cff3926588ae3d815744d1
Author: Dan Williams <djbw@kernel.org>

cxl: Limit CXL-CPER kfifo registration functions scope

This commit replaces EXPORT_SYMBOL_NS_GPL with EXPORT_SYMBOL_FOR_MODULES
for CXL-CPER kfifo registration functions to improve access control. It also
changes the register and unregister functions to return void and adds
synchronous cancellation during unregistration.

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6b..dd0a073af93c3 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -778,33 +778,34 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>  #endif
>  }
>  
> -int cxl_cper_register_prot_err_work(struct work_struct *work)
> +void cxl_cper_register_prot_err_work(struct work_struct *work)
>  {
> -	if (cxl_cper_prot_err_work)
> -		return -EINVAL;
> -
>  	guard(spinlock)(&cxl_cper_prot_err_work_lock);

Could this lead to a deadlock with the hardware error interrupt handler?

Looking at cxl_cper_post_prot_err() which is called from GHES error handling
in hard IRQ context, it uses the IRQ-safe variant to acquire the same lock:

	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);

Since guard(spinlock) in cxl_cper_register_prot_err_work() leaves local
interrupts enabled, if a hardware error interrupt occurs on the same CPU
while the lock is held here, the interrupt handler will execute
cxl_cper_post_prot_err() and spin indefinitely waiting for the lock.

Would it be better to use guard(spinlock_irqsave) here and in the unregister
function to prevent this lockup?

> +	WARN_ONCE(cxl_cper_prot_err_work,
> +		  "CPER-CXL kfifo consumer already registered\n");
>  	cxl_cper_prot_err_work = work;
> -	return 0;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_cper_register_prot_err_work, "CXL");
> +EXPORT_SYMBOL_FOR_MODULES(cxl_cper_register_prot_err_work, "cxl_core");
>  
> -int cxl_cper_unregister_prot_err_work(struct work_struct *work)
> +void cxl_cper_unregister_prot_err_work(void)
>  {
> -	if (cxl_cper_prot_err_work != work)
> -		return -EINVAL;
> +	struct work_struct *work;
>  
> -	guard(spinlock)(&cxl_cper_prot_err_work_lock);
> +	spin_lock(&cxl_cper_prot_err_work_lock);

Similarly, does this need to be spin_lock_irqsave() to protect against
the same deadlock scenario?

> +	work = cxl_cper_prot_err_work;
>  	cxl_cper_prot_err_work = NULL;
> -	return 0;
> +	spin_unlock(&cxl_cper_prot_err_work_lock);
> +
> +	if (work)
> +		cancel_work_sync(work);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505173029.2718246-1-terry.bowman@amd.com?part=5