Re: [PATCH v2 6/7] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion

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

Pre-existing issues:
- [High] Deadlock: `cxl_cper_register_prot_err_work` and `cxl_cper_unregister_prot_err_work` take `cxl_cper_prot_err_work_lock` without disabling interrupts, but the lock is also acquired in hardirq context.
- [High] `cxl_cper_prot_err_work_lock` is a `spinlock_t` acquired in hardirq context, which violates PREEMPT_RT locking rules.
- [High] Missing bounds check on `estatus->data_length` in `extlog_print` leads to out-of-bounds read of `elog_buf`.
--

--- Patch [6]: [PATCH v2 6/7] ACPI: extlog: Defer CXL protocol error handling to avoid lock inversion ---
commit 7b79585a4c6060a80ae775e5e2ed8715f3df690b
Author: Dave Jiang <[email protected]>

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

The commit avoids an AB-BA lock inversion between the device_lock and the
MCE notifier chain rwsem. It does this by deferring CXL protocol error
handling to a workqueue using cxl_cper_post_prot_err(), which posts errors
to a kfifo rather than handling them synchronously.

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

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 3e3e388af47b3..1fbaf134c04e3 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);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
estatus->data_length to cause an out-of-bounds read of the elog_buf?

Earlier in extlog_print(), estatus is copied into a 4096-byte (ELOG_ENTRY_LEN)
buffer:

    memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
    tmp = (struct acpi_hest_generic_status *)elog_buf;

Then apei_estatus_for_each_section(tmp, gdata) iterates over this memory
based on tmp->data_length. Since there appears to be no bounds check
verifying that data_length fits within ELOG_ENTRY_LEN, could a
firmware-controlled data_length cause the macro to read past the end of
the allocated buffer?

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 6fa5d3bab1ceb..b36f6d268c9fd 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 does cxl_cper_prot_err_work_lock cause
a sleeping in atomic violation on PREEMPT_RT kernels?

cxl_cper_prot_err_work_lock is declared as a standard spinlock_t using
DEFINE_SPINLOCK(), which becomes a sleepable rt_mutex on PREEMPT_RT. Since
cxl_cper_post_prot_err() is called from hardirq context (via
ghes_proc_in_irq), does acquiring this lock violate the rule against
acquiring sleepable locks in hardirq context?

[Severity: High]
This is a pre-existing issue, but is there a risk of a deadlock involving
cxl_cper_prot_err_work_lock?

When cxl_cper_register_prot_err_work() and cxl_cper_unregister_prot_err_work()
acquire this lock from process context, they use:

    guard(spinlock)(&cxl_cper_prot_err_work_lock);

This leaves interrupts enabled.

If a hardware error triggers an MCE or ghes_proc_in_irq on the same CPU while
the lock is held, cxl_cper_post_prot_err() will be called from hardirq
context. It will attempt to acquire the already-held lock using
guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock), which could freeze
the CPU.

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