Re: [PATCH v2 3/7] mtd: spi-nor: Add support for panic writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] pm: spi-mem: mishandled pm_runtime_get_if_active() silently breaks panic write
- [High] mm: spi-nor: panic write allocation uses sleeping GFP mask
- [High] spi: spi-mem: panic write fallback path sleeps on ENOTSUPP
--

commit a1f9f053470ff10fa10c133dc5aeb0827bc367a1
Author: Tanmay Jagdale <tanmay@marvell.com>

    mtd: spi-nor: Add support for panic writes
    
    If the SPI controller supports it, provide the functionality of doing a
    panic write.
    
    Update the spi-nor core code and controllers to make sure they won't try
    to grab locked mutexes or sleep when doing a panic write, as preemption
    and IRQs are disabled.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 2509fb983d4f2..281acc54b3245 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c

[ ... ]

> @@ -2196,6 +2235,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>  static int spi_nor_octal_dtr_write(struct spi_nor *nor, loff_t to, size_t len,
>  				   const u8 *buf)
>  {
> +	gfp_t mm_flags = GFP_KERNEL;
>  	u8 *tmp_buf;
>  	size_t bytes_written;
>  	loff_t start, end;
> @@ -2204,7 +2244,10 @@ static int spi_nor_octal_dtr_write(struct spi_nor *nor, loff_t to, size_t len,
>  	if (IS_ALIGNED(to, 2) && IS_ALIGNED(len, 2))
>  		return spi_nor_write_data(nor, to, len, buf);
>  
> -	tmp_buf = kmalloc(nor->params->page_size, GFP_KERNEL);
> +	if (nor->mtd.oops_panic_write)
> +		mm_flags |= GFP_ATOMIC;
> +
> +	tmp_buf = kmalloc(nor->params->page_size, mm_flags);

[Severity: High]
Does this bitwise OR successfully remove the sleepable __GFP_DIRECT_RECLAIM
flag?

Because mm_flags is initialized to GFP_KERNEL (which includes
__GFP_DIRECT_RECLAIM), bitwise ORing GFP_ATOMIC into the mask does not
clear the __GFP_DIRECT_RECLAIM bit. This means the resulting mask still
instructs the page allocator that it is allowed to sleep and enter direct
reclaim.

Since panic writes execute in a strictly atomic context with preemption and
local interrupts disabled, could this trigger a bug or deadlock and prevent
the oops log from being saved?

[ ... ]

> @@ -3589,8 +3652,13 @@ static int spi_nor_set_mtd_info(struct spi_nor *nor)
>  	mtd->size = nor->params->size;
>  	mtd->_read = spi_nor_read;
>  	/* Might be already set by some SST flashes. */
> -	if (!mtd->_write)
> +	if (!mtd->_write) {
>  		mtd->_write = spi_nor_write;
> +		if (nor->spimem &&
> +		    spi_mem_controller_is_capable(nor->spimem->spi->controller, panic_write)) {
> +			mtd->_panic_write = spi_nor_panic_write;
> +		}
> +	}

[Severity: Critical]
By hooking up panic write here, this delegates to spi_mem_exec_op() in
drivers/spi/spi-mem.c, which seems to mishandle the runtime pm reference
for panic writes.

In spi_mem_exec_op(), the panic write path does:

    else if (ctlr->auto_runtime_pm)
        ret = pm_runtime_get_if_active(ctlr->dev.parent);
    if (ret)
        return ret;

Since pm_runtime_get_if_active() returns 1 if the device is active, this
catches the success case (1) as an error and returns immediately, skipping
ctlr->mem_ops->exec_op() and leaking the pm reference.

Conversely, if the device was suspended, it returns 0, execution continues
on the suspended controller, and the subsequent pm_runtime_put_noidle() will
underflow the usage count.

Does this silently break the panic write execution?

[Severity: High]
Additionally, if the controller's exec_op() method dynamically returns
-ENOTSUPP or -EOPNOTSUPP, spi_mem_exec_op() falls back to a path that
cannot execute in atomic context:

    tmpbuf = kzalloc(tmpbufsize, GFP_KERNEL | GFP_DMA);
    ...
    ret = spi_sync(mem->spi, &msg);

Both kzalloc(GFP_KERNEL) and spi_sync() can sleep. If this fallback is
triggered during a panic write, will it violate atomic context constraints
and deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013141350.189996-1-paul.cercueil@bootlin.com?part=3

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/