Re: [PATCH v3 06/10] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting lock inversion (AA deadlock) between process context and IRQ context on CPER work locks.
- [Medium] extlog_print_pcie() is compiled out due to a misspelled preprocessor macro, rendering the function and its newly added bounds check dead code.
--

--- Patch [6]: [PATCH v3 06/10] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion ---
commit 004b48e5ad5ad98c6645296077a38ad36a959b26
Author: Dave Jiang <[email protected]>

ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

The commit exports cxl_cper_post_prot_err() from ghes.c and modifies
extlog_print() to use it. This avoids an AB-BA deadlock by deferring CXL
protocol error handling to a workqueue instead of calling it synchronously
while holding the MCE notifier chain rwsem.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 0c440d75d9a78..ae79d090de33b 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
[ ... ]
> @@ -244,9 +227,9 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  			struct cxl_cper_sec_prot_err *prot_err =
>  				acpi_hest_get_payload(gdata);
>  
> -			extlog_cxl_cper_handle_prot_err(prot_err,
> -							gdata->error_severity,
> -							gdata->error_data_length);
> +			cxl_cper_post_prot_err(prot_err,
> +					       gdata->error_severity,
> +					       gdata->error_data_length);
>  		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
>  			struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but does the extlog_print_pcie()
function called for this section type get entirely compiled out?

Looking at extlog_print_pcie() in drivers/acpi/acpi_extlog.c:

drivers/acpi/acpi_extlog.c:extlog_print_pcie() {
#ifdef ACPI_APEI_PCIEAER
    ...
}

Kernel Kconfig macros must use the CONFIG_ prefix to evaluate to true. Since
it checks for ACPI_APEI_PCIEAER instead of CONFIG_ACPI_APEI_PCIEAER, does this
silently disable PCIe AER error logging via the extlog driver and render the
newly added bounds check dead code?

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 17e4ef5552925..b8dbd99da47ef 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -752,8 +752,8 @@ static DEFINE_KFIFO(cxl_cper_prot_err_fifo, struct cxl_cper_prot_err_work_data,
>  static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
>  struct work_struct *cxl_cper_prot_err_work;
>  
> -static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -				   int severity, u32 len)
> +void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> +			    int severity, u32 len)
>  {

[Severity: High]
This is a pre-existing issue, but can this cause an AA deadlock?

Looking at cxl_cper_register_prot_err_work() and
cxl_cper_unregister_prot_err_work() in drivers/acpi/apei/ghes.c, they acquire
the cxl_cper_prot_err_work_lock using guard(spinlock) without disabling
interrupts since they execute in process context:

drivers/acpi/apei/ghes.c:cxl_cper_register_prot_err_work() {
    ...
    guard(spinlock)(&cxl_cper_prot_err_work_lock);
    ...
}

If an asynchronous hardware error interrupt fires on the same CPU while this
lock is held, the interrupt handler calls cxl_cper_post_prot_err() which
attempts to acquire the exact same lock using guard(spinlock_irqsave):

drivers/acpi/apei/ghes.c:cxl_cper_post_prot_err() {
    ...
    guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
    ...
}

Would this cause the interrupt handler to spin forever waiting for the lock
that the interrupted process context currently holds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6