[PATCH v3] mtd: spi-nor: take the flash lock around spi_nor_restore()

[PATCH v3] mtd: spi-nor: take the flash lock around spi_nor_restore()

[Itai Handler]

spi_nor_shutdown() and spi_nor_remove() call spi_nor_restore() without
nor->lock, which every other path to the chip takes through
spi_nor_prep_and_lock(). Both run with the MTD device still registered,
so another thread can be in the middle of an operation.

A busy flash ignores everything but status reads, so the restore is
silently dropped and the chip is left in 4-byte mode. A restore landing
between two chunks of a read switches the chip to 3-byte addressing
while spi_nor_read() carries on sending four address bytes.

Take the lock, so the restore runs between operations instead of during
one. This narrows the race rather than closing it: an operation starting
afterwards still addresses a 3-byte chip with nor->addr_nbytes left at 4.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
v2 1/3, the spi_nor_rww_start_exclusive() lock fix, is dropped from this
series now that it is queued in spi-nor/next. This patch still depends
on it: it adds the first ->shutdown and ->remove callers of the
exclusive lock, so an RWW flash would deadlock without it. Please let it
reach stable first.

Changes in v3:
- Fold the spi_nor_remove() patch into this one, as requested by
  Michael Walle. Both call sites now share a small helper.
- Cut the commit message down.
- Add an Assisted-by: tag, as requested by Michael Walle.
- Drop patch 1/3, queued in spi-nor/next.
- Link to v2:
  https://lore.kernel.org/r/20260914081149.1916589-1-itai.handler@gmail.com

 drivers/mtd/spi-nor/core.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ccf4396cdcd0..c891197ffa5b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3849,11 +3849,26 @@ static int spi_nor_probe(struct spi_mem *spimem)
 				   data ? data->nr_parts : 0);
 }
 
+/*
+ * Restore between operations, not during one. Removal and shutdown both run
+ * with MTD users still attached: a busy flash silently ignores the commands
+ * spi_nor_restore() sends, and a restore landing inside a read changes the
+ * chip's address width under the transfer.
+ */
+static void spi_nor_restore_locked(struct spi_nor *nor)
+{
+	if (spi_nor_prep_and_lock(nor))
+		return;
+
+	spi_nor_restore(nor);
+	spi_nor_unlock_and_unprep(nor);
+}
+
 static int spi_nor_remove(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
-	spi_nor_restore(nor);
+	spi_nor_restore_locked(nor);
 
 	/* Clean up MTD stuff. */
 	return mtd_device_unregister(&nor->mtd);
@@ -3863,7 +3878,7 @@ static void spi_nor_shutdown(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
-	spi_nor_restore(nor);
+	spi_nor_restore_locked(nor);
 }
 
 /*

base-commit: 50d05c7c76c96b90462f24debacca971d2e86713
-- 
2.34.1

Re: [PATCH v3] mtd: spi-nor: take the flash lock around spi_nor_restore()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: spi-nor: skipped flash restore on fatal signal breaks boot
- [High] mtd: spi-nor: shutdown hang due to ongoing erase operation
--

commit c0578fac7581b63c39bfd99ac3a6f934256685d3
Author: Itai Handler <itai.handler@gmail.com>

mtd: spi-nor: take the flash lock around spi_nor_restore()

This commit changes spi_nor_shutdown() and spi_nor_remove() to acquire
nor->lock around spi_nor_restore(). It aims to prevent operations like
reads from overlapping with the restore, which could leave the chip in
3-byte addressing mode.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ccf4396cdcd04..c891197ffa5b3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3849,11 +3849,26 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  				   data ? data->nr_parts : 0);
>  }
>  
> +/*
> + * Restore between operations, not during one. Removal and shutdown both run
> + * with MTD users still attached: a busy flash silently ignores the commands
> + * spi_nor_restore() sends, and a restore landing inside a read changes the
> + * chip's address width under the transfer.
> + */
> +static void spi_nor_restore_locked(struct spi_nor *nor)
> +{
> +	if (spi_nor_prep_and_lock(nor))
> +		return;

[Severity: High]
Does this code silently skip the flash restore if a fatal signal breaks the
lock acquisition? 

For example, if a privileged user runs rmmod while an erase or read operation
is actively holding the RWW lock, and the rmmod process receives SIGKILL,
spi_nor_prep_and_lock() (via wait_event_killable()) will return -ERESTARTSYS:

drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
    ...
    if (!spi_nor_use_parallel_locking(nor))
        mutex_lock(&nor->lock);
    else
        ret = wait_event_killable(nor->rww.wait,
                                  spi_nor_rww_start_exclusive(nor));

    return ret;
}

Bypassing the hardware teardown path here leaves the flash in 4-byte addressing
mode. This might cause the bootloader to fail to read the kernel on the next
reboot, soft-bricking the device until a power cycle.

> +
> +	spi_nor_restore(nor);
> +	spi_nor_unlock_and_unprep(nor);
> +}
> +

[ ... ]

> @@ -3863,7 +3878,7 @@ static void spi_nor_shutdown(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
> -	spi_nor_restore(nor);
> +	spi_nor_restore_locked(nor);

[Severity: High]
Could this hang the system indefinitely during shutdown if an erase operation
is ongoing?

Erase operations (like spi_nor_erase_multi_sectors) hold nor->lock
continuously for their entire duration, looping over spi_nor_wait_till_ready()
with cond_resched(). A large chip erase can take several minutes.

Because device_shutdown() iterates devices synchronously without a timeout,
this synchronous wait for nor->lock could hang the reboot process until the
erase finishes, forcing the user to hard-reset the machine.

>  }
>  
>  /*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915113811.2429311-1-itai.handler@gmail.com?part=1