Re: [PATCH RFC v3 6/7] mfd: ls2kbmc: Able to be compiled as a module

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

Pre-existing issues:
- [Critical] An explicit 10-second delay is executed inside a stop_machine() context, halting the system and triggering the hard lockup detector.
- [High] Dynamic amendment of a global static, non-const mfd_cell array with a stack-allocated variable leaves a dangling pointer.
- [High] Premature disposal of the GPIO IRQ mapping by calling acpi_unregister_gsi() immediately after requesting it.
--

--- Patch [6]: [PATCH RFC v3 6/7] mfd: ls2kbmc: Able to be compiled as a module ---
commit d873614dbdbdbcb510f15d96f0137b762016fd6d
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Able to be compiled as a module

This commit changes the MFD_LS2K_BMC_CORE Kconfig option from a boolean
to a tristate. This allows the Loongson-2K Board Management Controller
driver to be built and loaded dynamically as a kernel module.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782b..a7a9f97af248c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2494,7 +2494,7 @@ config MFD_LOONGSON_SE
>  	  which will forward them to the corresponding engine.
>  
>  config MFD_LS2K_BMC_CORE
> -	bool "Loongson-2K Board Management Controller Support"
> +	tristate "Loongson-2K Board Management Controller Support"
>  	depends on PCI && ACPI_GENERIC_GSI
>  	select MFD_CORE
>  	help

Since this patch enables module unloading for the ls2k-bmc driver, I reviewed
the existing driver code and found a few pre-existing regressions that are
impacted by or related to this change.

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

[Severity: High]
This isn't a bug introduced by this patch, but does this dynamically amend a
global non-const mfd_cell array with a stack-allocated variable?

Because ls2k_bmc_cells is a shared global static array, if two BMC devices
are probed concurrently, thread A and thread B could race to write their local
stack pointers into this global array. Thread A might pass thread B's stack
pointer to devm_mfd_add_devices(). 

If thread B has already returned, could thread A read from a popped stack
frame and yield garbage resolution data?

Does this also violate the MFD subsystem rule that mfd_cell arrays must be
static const?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this prematurely dispose of the GPIO
IRQ mapping?

In ls2k_bmc_init(), acpi_register_gsi() maps an IRQ and devm_request_irq()
requests it. Immediately after, acpi_unregister_gsi() is called on the exact
same IRQ number. This invokes irq_dispose_mapping(), which tears down the
IRQ descriptor while the action is still linked.

Since this patch turns the driver into a module, module unloading is now
possible. When the module is unloaded, devm_irq_release() will fire on the
disposed IRQ. 

Could this lead to a use-after-free or inadvertently free an unrelated IRQ
that reused the same descriptor?

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);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this execute a 10-second delay inside
a stop_machine() context?

The stop_machine() function halts all CPUs in the system with local interrupts
disabled to serialize execution. Inside ls2k_bmc_recover_pci_data(), there is
an unconditional mdelay() for 10 seconds.

Could spinning a CPU with interrupts disabled for over 10 seconds trigger a
hard lockup panic and disrupt system timekeeping and networking?

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