Re: [PATCH RFC v6 3/7] mfd: ls2kbmc: Redraw using exported functions

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

Pre-existing issues:
- [Critical] Excessive busy-waiting with IRQs disabled in `stop_machine` callback causes hard lockups.
- [High] Global `mfd_cell` array is dynamically mutated using a local stack pointer during probe.
- [High] Missing `cancel_work_sync` on driver unbind leads to use-after-free of `ddata` by the workqueue.
- [High] Ignored failure of PCI recovery leads to kernel crash when updating screen.
--

--- Patch [3]: [PATCH RFC v6 3/7] mfd: ls2kbmc: Redraw using exported functions ---
commit 9d8a26242ae07ab2514af3ffede82d5bccaaf8a9
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Redraw using exported functions

This commit replaces the manual virtual terminal relocation with a call to
update_screen() to trigger a redraw of the current VT after a BMC reset.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index a9a30611dde9e..e12f2ca2fbef3 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -25,6 +25,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/stop_machine.h>
>  #include <linux/vt_kern.h>
> +#include <linux/console.h>
>  
>  /* LS2K BMC resources */
>  #define LS2K_DISPLAY_RES_START		(SZ_16M + SZ_2M)
> @@ -310,7 +311,9 @@ static void ls2k_bmc_events_fn(struct work_struct *work)
>  
>  	if (IS_ENABLED(CONFIG_VT)) {
>  		/* Re-push the display due to previous PCI-E loss. */

[Severity: Critical]
This is a pre-existing issue, but can the stop_machine() call that precedes 
this block cause a hard lockup?

In ls2k_bmc_events_fn(), stop_machine() is called to execute 
ls2k_bmc_recover_pci_data():

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

During stop_machine(), local interrupts are disabled. However, 
ls2k_bmc_recover_pci_data() performs massive busy waits:

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

With mdelay() loops of up to 12 seconds total while interrupts are disabled, 
could this trigger an nmi watchdog timeout panic?


[Severity: High]
This isn't a bug introduced by this patch, but does this code handle PCI 
recovery failures correctly?

Looking at ls2k_bmc_recover_pci_data(), it returns 0 both on success and when 
it fails:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
	if (i == 0)
		return false;
...
	return 0;
}

Because stop_machine() masks the error by propagating this 0, 
update_screen() will be called here unconditionally.

If the PCI link fails to recover, could writing to the unmapped framebuffer 
trigger a synchronous external bus error and crash the system?

> -		set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
> +		console_lock();
> +		update_screen(vc_cons[fg_console].d);
> +		console_unlock();
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race when 
setting up the mfd_cell array during probe?

In ls2k_bmc_probe(), the global ls2k_bmc_cells array is dynamically mutated 
using a pointer to a local stack variable:

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

Since ls2k_bmc_cells is a global static array without a const qualifier:

drivers/mfd/ls2k-bmc-core.c {
static struct mfd_cell ls2k_bmc_cells[] = {
...
}

Does this leave a dangling stack pointer in a global array if multiple 
devices are probed or if the array is referenced later?


[Severity: High]
This is a pre-existing issue, but does this driver safely tear down the 
reset workqueue on unbind?

The bmc_reset_work is initialized in ls2k_bmc_init(), but the driver lacks 
a remove hook to cancel it:

drivers/mfd/ls2k-bmc-core.c {
static struct pci_driver ls2k_bmc_driver = {
	.name = "ls2k-bmc",
	.id_table = ls2k_bmc_devices,
	.probe = ls2k_bmc_probe,
};
...

If the driver unbinds or probe fails, devres frees the irq and ddata.

If the work is already scheduled or executing when this happens, could the 
workqueue dereference the freed ddata memory?

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