Re: [PATCH RFC v8 4/8] mfd: ls2kbmc: Cancel the work queue on removal

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

Pre-existing issues:
- [Critical] A 10-second busy-wait delay and sleepable PCI mapping functions are executed within a `stop_machine()` context, causing system lockups.
- [Critical] The shared PCI interrupt handler blindly schedules the BMC reset recovery work without verifying the interrupt source, causing spurious 10-second system stalls on any shared IRQ.
- [High] The global `ls2k_bmc_cells` MFD array is dynamically mutated with a stack pointer, violating subsystem rules and causing memory safety hazards.
- [High] The GSI mapping for the GPIO watchdog interrupt is unconditionally destroyed immediately after being requested, breaking the interrupt.
- [High] A static zero-initialized variable for rate limiting causes legitimate interrupts to be permanently ignored during the first 6 minutes of uptime.
--

--- Patch [4]: [PATCH RFC v8 4/8] mfd: ls2kbmc: Cancel the work queue on removal ---
commit ac625d16d31c097a08a5eeba96a345ec9e2f3716
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Cancel the work queue on removal

This commit adds a devm action to cancel the bmc_reset_work when the
device is removed. This prevents pending work from executing after the
device has been unbound.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 06ec8143d8cdb..5eea53f134215 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -375,6 +375,12 @@ static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *
>  	pci_read_config_dword(pdev, PCI_INTERRUPT_LINE, &ddata->bmc_pci_data.interrupt_line);
>  }
>  
> +static void ls2k_bmc_cancel_wq(void *data)
> +{
> +	struct ls2k_bmc_ddata *ddata = data;
> +	(void) cancel_work_sync(&ddata->bmc_reset_work);
> +}
> +
>  static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>  {
>  	struct pci_dev *pdev = to_pci_dev(ddata->dev);
> @@ -385,6 +391,10 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>  
>  	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
>  
> +	ret = devm_add_action_or_reset(ddata->dev, ls2k_bmc_cancel_wq, ddata);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
>  			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", ddata);
>  	if (ret) {

[Severity: High]
This is a pre-existing issue, but the GSI mapping for the GPIO watchdog
interrupt is unconditionally destroyed immediately after being requested:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    gpio_irq = acpi_register_gsi(NULL, LS2K_BMC_RESET_GPIO_GSI, ACPI_EDGE_SENSITIVE,
                                 ACPI_ACTIVE_LOW);
    ...
    ret = devm_request_irq(ddata->dev, gpio_irq, ls2k_bmc_interrupt,
                           IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc gpio", ddata);
    ...
    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    return ret;
}

Since this executes irq_dispose_mapping(), won't this cause the registered
handler to never trigger and potentially lead to a use-after-free or corrupted
interrupts on removal?

[Severity: Critical]
This is a pre-existing issue, but does the shared PCI interrupt handler
blindly schedule the BMC reset recovery work without verifying the interrupt
source?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    ...
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
        schedule_work(&ddata->bmc_reset_work);
        last_jiffies = jiffies;
    }

    return IRQ_HANDLED;
}

Since the PCI IRQ is requested with IRQF_SHARED, could any interrupt from
another device sharing the PCI IRQ line spuriously trigger the BMC recovery
work?

Since this work executes stop_machine() with a 10-second busy loop,
won't normal activity on shared devices freeze the entire system for 10
seconds?

[Severity: High]
This is a pre-existing issue, but does a static zero-initialized variable
for rate limiting cause legitimate interrupts to be permanently ignored
during early boot?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    ...
    static unsigned long last_jiffies;
    ...
    /* Skip interrupt in LS2K_BMC_INT_INTERVAL */
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
    ...
}

Because the Linux jiffies variable starts at 5 minutes before wrap-around,
time_after(jiffies, 0 + 60*HZ) evaluates to false for the first 6 minutes
of system uptime. 

Will this cause hardware recovery interrupts to be completely ignored during 
this time, and multiple device instances to interfere with each other's 
rate-limiting state?

[Severity: Critical]
This is a pre-existing issue, but are a 10-second busy-wait delay and
sleepable PCI mapping functions executed within a stop_machine() context?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn() {
    ...
    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
    ...
}

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

Will executing a 10-second busy-wait with hard interrupts disabled across
all CPUs trigger the NMI watchdog and crash the system? 

Additionally, since ls2k_bmc_recover_pci_data() calls pci_iomap() via 
ls2k_bmc_pcie_is_connected(), won't invoking sleepable memory allocations 
and locks in an atomic context cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but is the global ls2k_bmc_cells MFD array
dynamically mutated with a stack pointer?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe() {
    ...
    struct simplefb_platform_data pd;
    ...
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);
    ...
}

According to MFD subsystem guidelines, mfd_cell arrays must be static const.

Does modifying a global array with a stack pointer create a use-after-free
risk and a concurrency race if multiple instances are probed, since it leaves
the global array permanently holding a dangling stack pointer after probe()
returns?

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