Re: [PATCH v3 3/3] MIPS: Octeon: flash: use the simple map accessors without a shared eMMC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: cavium: global state corruption of flash_map on multiple probes
--

commit c32cf6c857e3356732452178a52a4e739fcaf273
Author: Orgad Shaneh <orgads@gmail.com>

MIPS: Octeon: flash: use the simple map accessors without a shared eMMC

This commit modifies the Octeon flash driver to conditionally use simple map
accessors when the bootbus is not shared with an eMMC host. Doing so avoids
unnecessary semaphore usage and enables MTD to use the point() method
for faster jffs2 mounts. It also adds early bank width validation before
mapping memory.

> diff --git a/arch/mips/cavium-octeon/flash_setup.c b/arch/mips/cavium-octeon/flash_setup.c
> index 3395acde4d60a..5448d5139c640 100644
> --- a/arch/mips/cavium-octeon/flash_setup.c
> +++ b/arch/mips/cavium-octeon/flash_setup.c

[ ... ]

> @@ -102,11 +119,20 @@ static int octeon_flash_probe(struct platform_device *pdev)
>  		flash_map.virt = ioremap(flash_map.phys, flash_map.size);
>  		pr_notice("Bootbus flash: Setting flash for %luMB flash at "
>  			  "0x%08llx\n", flash_map.size >> 20, flash_map.phys);
> -		WARN_ON(!map_bankwidth_supported(flash_map.bankwidth));
> -		flash_map.read = octeon_flash_map_read;
> -		flash_map.write = octeon_flash_map_write;
> -		flash_map.copy_from = octeon_flash_map_copy_from;
> -		flash_map.copy_to = octeon_flash_map_copy_to;
> +		/*
> +		 * The boot bus semaphore only serializes the flash against
> +		 * an eMMC host that shares the bus (the CIU3 parts have
> +		 * their own). Without one, use the simple accessors: they
> +		 * let MTD point at the flash, so jffs2 scans it in place.
> +		 */
> +		if (octeon_flash_bus_shared()) {
> +			flash_map.read = octeon_flash_map_read;
> +			flash_map.write = octeon_flash_map_write;
> +			flash_map.copy_from = octeon_flash_map_copy_from;
> +			flash_map.copy_to = octeon_flash_map_copy_to;
> +		} else {
> +			simple_map_init(&flash_map);
> +		}
>  		mymtd = do_map_probe("cfi_probe", &flash_map);

[Severity: High]
Can this cause global state corruption if multiple flash devices are probed?

Looking at arch/mips/cavium-octeon/flash_setup.c, flash_map is defined as a
single static struct map_info instance:

static struct map_info flash_map;

If multiple cfi-flash devices are present on the platform bus,
octeon_flash_probe() will be called for each of them. A second probe will
overwrite the phys, virt, size, and bankwidth fields of the shared flash_map
instance.

Since do_map_probe() passes a pointer to flash_map to the CFI layer, both
mtd_info instances will retain a reference to this single map via mtd->priv. Any
I/O on the first flash device would then use the mapped memory, size, and CFI
state of the second device, which could lead to data corruption.

Does it make sense to allocate struct map_info dynamically per device instead?

>  		if (mymtd) {
>  			mymtd->owner = THIS_MODULE;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011062908.2365879-1-orgads@gmail.com?part=3