[PATCH v2 0/3] mtd: spi-nor: fix the unlocked restore on shutdown and remove

[PATCH v2 0/3] mtd: spi-nor: fix the unlocked restore on shutdown and remove

[Itai Handler]

Two threads can talk to the flash at once during reboot/kexec and during
an unbind, because spi_nor_restore() is called without nor->lock while
MTD users are still attached. A busy flash silently ignores the restore
and is left in 4-byte addressing, which is exactly the failure the
restore was added to prevent; a restore landing inside a read corrupts
the rest of that read instead.

I reproduced this under QEMU, using a flash model extended to implement
erase busy time. With an erase outstanding, spi_nor_shutdown() issues
WREN, EX4B and WRDI; the chip refuses all three because it is busy, and
each one still reports success to the caller:

  the flash was mid-erase when spi_nor_shutdown() tried to put it back
  into 3-byte addressing, and refused 3 of its command(s):
  0x06 (four_byte=1), 0xe9 (four_byte=1), 0x04 (four_byte=1)

four_byte=1 is the mode the chip was left in, and so the mode the next
kernel inherits. With the patch, shutdown waits for the erase and the
restore reaches an idle chip. The reproduction is deterministic: the
workload keeps the flash busy continuously, so no timing window is
involved.

Two caveats on that. It runs on a 5.10 vendor tree rather than mainline,
though the path is unchanged - mainline's spi_nor_shutdown() has the
same unlocked spi_nor_restore() call. And what started the investigation
was intermittent hangs after kexec on a Zynq UltraScale+ board, which I
have not tied to this race.

Patch 1 is new in v2 and is a prerequisite rather than part of the fix.
spi_nor_rww_start_exclusive() returns with nor->lock held on both of its
exits, so taking the flash lock in ->shutdown and ->remove, which is
what patches 2 and 3 do, would deadlock an RWW flash on every reboot and
every unbind. Nothing reaches that code today, which is how it survived
since v6.15, so patch 1 also stands on its own.

Patch 2 fixes ->shutdown, which every reboot and kexec goes through, and
is marked for stable. Patch 3 fixes the identical problem in ->remove; I
have deliberately not marked it for stable, since nobody has reported
hitting it and it changes how long an unbind can block. Patch 1 carries
a stable tag too, so that a backport of patch 2 cannot land without it.

Note what patch 2 does not do: the restore still runs with MTD users
attached, so an operation starting after it completes still addresses a
3-byte chip with nor->addr_nbytes == 4. Serialising against operations
already in flight is what stops the restore being issued into a busy
chip; fully closing the window would mean stopping MTD from accepting
operations before ->shutdown, which seemed too big a change to fold in
here. I am happy to look at that separately if you would prefer it.

Patches 2 and 3 are independent of each other; patch 3 can be dropped
without affecting patch 2. Patch 1 has to stay.

Changes in v2:
- New patch 1/3 fixing the lock that spi_nor_rww_start_exclusive()
  leaves held on both exits. Without it the rest of the series
  deadlocks on an RWW flash, because it adds the first ->shutdown and
  ->remove callers of the exclusive lock. Found while answering the
  automated review of v1.
- Patches 2/3 and 3/3 are unchanged from v1 1/2 and 2/2.
- Link to v1:
  https://lore.kernel.org/r/20260910184452.895485-1-itai.handler@gmail.com

Itai Handler (3):
  mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()
  mtd: spi-nor: take the flash lock in spi_nor_shutdown()
  mtd: spi-nor: take the flash lock in spi_nor_remove()

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


base-commit: 50d05c7c76c96b90462f24debacca971d2e86713
-- 
2.34.1

[PATCH v2 1/3] mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()

[Itai Handler]

spi_nor_rww_start_exclusive() takes nor->lock and never drops it. It
returns with the mutex held whether it hands out the exclusive claim or
reports the flash busy, leaving the caller holding a lock it does not
know it has.

Commit 03e7bb864d9a ("mtd: spi-nor: use scope-based mutex cleanup
helpers") turned its "goto busy" into a plain "return false" and deleted
the busy: label that did the mutex_unlock(), but kept the mutex_lock()
at the top instead of replacing it with guard(mutex). It is now the only
one of the ten spi_nor_rww_{start,end}_* helpers that does not use the
guard.

Its only caller is spi_nor_prep_and_lock(), so on a flash with
SNOR_F_RWW set:

 - if the flash is idle it returns true with nor->lock held, and the
   matching spi_nor_unlock_and_unprep() calls
   spi_nor_rww_end_exclusive(), whose guard(mutex)(&nor->lock) then
   deadlocks on the non-recursive mutex;

 - if the flash is busy it returns false with nor->lock held, and
   wait_event_killable() sleeps holding it, so the operation that would
   clear ongoing_* can never take the lock to do so.

Nothing reaches this today: the only flash with SPI_NOR_RWW is the
MX25UW51245G, which has neither OTP nor locking ops, so none of the
existing spi_nor_prep_and_lock() callers in otp.c, swp.c and sst.c apply
to it. It becomes reachable as soon as any common path takes the
exclusive lock.

Use guard(mutex) as the other helpers do.

Fixes: 03e7bb864d9a ("mtd: spi-nor: use scope-based mutex cleanup helpers")
Cc: stable@vger.kernel.org
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ccf4396cdcd0..8bc117b46e02 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1310,7 +1310,7 @@ static bool spi_nor_rww_start_exclusive(struct spi_nor *nor)
 {
 	struct spi_nor_rww *rww = &nor->rww;
 
-	mutex_lock(&nor->lock);
+	guard(mutex)(&nor->lock);
 
 	if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
 		return false;
-- 
2.34.1

[PATCH v2 2/3] mtd: spi-nor: take the flash lock in spi_nor_shutdown()

[Itai Handler]

spi_nor_shutdown() calls spi_nor_restore() to put the flash back into
3-byte addressing before the system reboots or kexecs. It does so
without taking nor->lock, which every other path that talks to the chip
acquires through spi_nor_prep_and_lock().

device_shutdown() does not freeze userspace and does not stop kernel
threads; it walks the device list calling ->shutdown with all CPUs
online. Another thread can therefore be in the middle of an operation,
with the restore running concurrently with it. A write and a read are
both damaged, in different ways.

A program or erase leaves the flash busy, and a busy flash accepts only
status register reads and ignores everything else, including the EX4B
that spi_nor_restore() sends. Neither spi_nor_write_enable() nor
spi_nor_set_4byte_addr_mode() reads anything back, so the restore
reports success while the flash is left in 4-byte addressing. The next
boot stage then addresses it with 3 bytes and reads the wrong data,
which is the failure commit 59b356ffd0b0 ("mtd: m25p80: restore the
status of SPI flash when exiting") introduced this restore to prevent.

A read, by contrast, does not ignore the restore - it is corrupted by
it. spi_nor_read() holds the lock across a loop that issues one
spi_nor_read_data() per chunk, each using nor->addr_nbytes.
spi_nor_set_4byte_addr_mode() updates nor->params->addr_nbytes and not
nor->addr_nbytes, so a restore landing between two chunks switches the
chip to 3-byte addressing while the driver carries on sending 4 address
bytes. The rest of the transfer is addressed wrongly and returns wrong
data, and nothing reports an error. A restore may also soft reset the
chip in the middle of that same read.

Take nor->lock for the restore, so it runs between operations instead of
during one: a program or erase has finished waiting on the chip, and a
read has issued its last chunk. This is a locking fix rather than a
missing wait - each operation already waits for completion at the site
that started it.

This narrows the race without closing it. The restore still runs while
MTD users are attached, so an operation that starts after it has
completed will address a chip that is now in 3-byte mode while
nor->addr_nbytes is still 4. Closing that as well would mean having MTD
stop accepting operations before ->shutdown runs, which is a larger
change; serialising against the operations already in flight is what
keeps the restore itself from being issued into a busy chip.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Cc: stable@vger.kernel.org
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 8bc117b46e02..647bf8dce719 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3862,8 +3862,21 @@ static int spi_nor_remove(struct spi_mem *spimem)
 static void spi_nor_shutdown(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+	int ret;
+
+	/*
+	 * Wait for an operation started by another thread to finish.
+	 * device_shutdown() runs with MTD users still active: a busy flash
+	 * ignores the commands spi_nor_restore() issues, leaving it in
+	 * 4-byte address mode, and a restore landing mid-read changes the
+	 * chip's address width under the transfer.
+	 */
+	ret = spi_nor_prep_and_lock(nor);
+	if (ret)
+		return;
 
 	spi_nor_restore(nor);
+	spi_nor_unlock_and_unprep(nor);
 }
 
 /*
-- 
2.34.1

[PATCH v2 3/3] mtd: spi-nor: take the flash lock in spi_nor_remove()

[Itai Handler]

spi_nor_remove() restores the addressing mode with the same unlocked
call to spi_nor_restore() that spi_nor_shutdown() used before the
previous patch, and it is exposed the same way: the MTD device is still
registered at that point, so an unbind can run the restore while another
thread is in the middle of an operation. A busy flash silently ignores
the restore, and a restore that lands between two chunks of a read
switches the chip to 3-byte addressing while the driver keeps sending
4 address bytes.

Moving the restore after mtd_device_unregister() would not fix this.
Since commit 19bfa9ebebb5 ("mtd: use refcount to prevent corruption")
del_mtd_device() drops a reference instead of refusing with -EBUSY when
the device is in use, so unregistering returns right away and does not
wait for an operation that is already running.

Take nor->lock for the restore, as spi_nor_shutdown() now does. The
unregister stays unconditional, so a flash whose restore had to be
skipped is still torn down.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 647bf8dce719..8d0302565445 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3852,8 +3852,14 @@ static int spi_nor_probe(struct spi_mem *spimem)
 static int spi_nor_remove(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
+	int ret;
 
-	spi_nor_restore(nor);
+	/* As in spi_nor_shutdown(), do not restore under an operation. */
+	ret = spi_nor_prep_and_lock(nor);
+	if (!ret) {
+		spi_nor_restore(nor);
+		spi_nor_unlock_and_unprep(nor);
+	}
 
 	/* Clean up MTD stuff. */
 	return mtd_device_unregister(&nor->mtd);
-- 
2.34.1

Re: [PATCH v2 1/3] mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing spi_nor_unprep() on error path when wait_event_killable() fails in spi_nor_prep_and_lock() and its variants.
--

commit c817801772cd87c9d5fe101e67f14bf0d06c97df
Author: Itai Handler <itai.handler@gmail.com>
Subject: mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()

This commit changes spi_nor_rww_start_exclusive() to use scope-based
mutex cleanup helpers instead of manually locking without unlocking.
This fixes a deadlock and allows operations to proceed when the flash
is busy by ensuring the mutex is released upon returning.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ccf4396cdcd04..8bc117b46e028 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1310,7 +1310,7 @@ static bool spi_nor_rww_start_exclusive(struct spi_nor *nor)
>  {
>  	struct spi_nor_rww *rww = &nor->rww;
>  
> -	mutex_lock(&nor->lock);
> +	guard(mutex)(&nor->lock);
>  
>  	if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
>  		return false;

[Severity: High]
This is a pre-existing issue, but while reviewing the wait condition here,
I noticed a missing cleanup path in spi_nor_prep_and_lock() and its program,
erase, and read variants.

Looking at spi_nor_prep_and_lock() in drivers/mtd/spi-nor/core.c, it starts
by calling spi_nor_prep() which acquires resources like a hardware mutex and
clock. When parallel locking is enabled, it calls wait_event_killable():

drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
    ...
	ret = spi_nor_prep(nor);
	if (ret)
		return ret;

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

If wait_event_killable() is interrupted by a fatal signal, it returns
-ERESTARTSYS. Does this directly return the error and skip spi_nor_unprep()?

Since the controller's .prepare hook acquires hardware resources, would
returning directly here cause a permanent deadlock on the next access and a
resource leak?

Userspace triggering I/O on an RWW flash can be signaled while waiting for
ongoing I/O to finish, so this error path seems reachable. 

Does this same pattern appear in spi_nor_prep_and_lock_pe() using
spi_nor_rww_start_pe() and in spi_nor_prep_and_lock_rd() using
spi_nor_rww_start_rd()?

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

Re: [PATCH v2 2/3] mtd: spi-nor: take the flash lock in spi_nor_shutdown()

[sashiko-bot]

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

New issues:
- [High] Returning early from `spi_nor_shutdown()` when `spi_nor_prep_and_lock()` is interrupted by a signal skips critical hardware restoration, potentially bricking the device on next boot.

Pre-existing issues:
- [High] Missing `spi_nor_unprep()` on the error path of `spi_nor_prep_and_lock()` and its variants causes controller resource leaks and potential deadlocks.
--

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

mtd: spi-nor: take the flash lock in spi_nor_shutdown()

This commit acquires the flash lock in spi_nor_shutdown() to serialize
device restoration with concurrent operations. This prevents the restore
command from being ignored by a busy flash chip or corrupting an ongoing
read operation during system shutdown.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8bc117b46e028..647bf8dce7193 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3862,8 +3862,21 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  static void spi_nor_shutdown(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	int ret;
> +
> +	/*
> +	 * Wait for an operation started by another thread to finish.
> +	 * device_shutdown() runs with MTD users still active: a busy flash
> +	 * ignores the commands spi_nor_restore() issues, leaving it in
> +	 * 4-byte address mode, and a restore landing mid-read changes the
> +	 * chip's address width under the transfer.
> +	 */
> +	ret = spi_nor_prep_and_lock(nor);
> +	if (ret)
> +		return;

[Severity: High]
If wait_event_killable() in spi_nor_prep_and_lock() is interrupted by a fatal
signal, it returns -ERESTARTSYS. Will returning early here skip the critical
hardware restoration in spi_nor_restore()?

If the flash chip is left in 4-byte addressing mode, could the bootloader
read garbage data on the next boot, effectively soft-bricking the device
until a hard power cycle?

[Severity: High]
This isn't a bug introduced by this patch, but does spi_nor_prep_and_lock()
leak controller resources on its error path?

Looking at spi_nor_prep_and_lock() in drivers/mtd/spi-nor/core.c:

int spi_nor_prep_and_lock(struct spi_nor *nor)
{
	int ret;

	ret = spi_nor_prep(nor);
	if (ret)
		return ret;

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

If wait_event_killable() is interrupted and returns an error, the function
returns without calling spi_nor_unprep(nor). Could this permanently leak the
hardware locks and clocks enabled in spi_nor_prep(), deadlocking the
controller for all subsequent SPI operations?

The same behavior appears to be present in the spi_nor_prep_and_lock_pe() and
spi_nor_prep_and_lock_rd() variants.

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

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

Re: [PATCH v2 3/3] mtd: spi-nor: take the flash lock in spi_nor_remove()

[sashiko-bot]

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

New issues:
- [Critical] Using a killable wait in the device removal path allows signals to abort critical hardware teardown, potentially bricking the device on the next boot.

Pre-existing issues:
- [Critical] devm-allocated driver data (struct spi_nor and its DMA bouncebuf) is freed while userspace can still access the MTD device.
- [High] Releasing the device lock before unregistering the MTD device exposes restored hardware to concurrent I/O and block-layer flushes, causing data corruption.
- [High] If `wait_event_killable()` is interrupted by a signal, `spi_nor_prep_and_lock()` returns an error without unwinding `spi_nor_prep()`, permanently leaking the SPI controller's prepared state.
--

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

mtd: spi-nor: take the flash lock in spi_nor_remove()

spi_nor_remove() restores the addressing mode with the same unlocked
call to spi_nor_restore() that spi_nor_shutdown() used before the
previous patch, and it is exposed the same way. This patch moves the
restore inside spi_nor_prep_and_lock() to ensure the lock is held.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 647bf8dce7193..8d0302565445b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3852,8 +3852,14 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  static int spi_nor_remove(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	int ret;
>  
> -	spi_nor_restore(nor);
> +	/* As in spi_nor_shutdown(), do not restore under an operation. */
> +	ret = spi_nor_prep_and_lock(nor);

[Severity: High]
This is a pre-existing issue, but does this function leak the SPI
controller's prepared state if interrupted by a signal?

Looking at drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock(), it
successfully calls spi_nor_prep(), but then waits via wait_event_killable():

	ret = wait_event_killable(nor->rww.wait,
				  spi_nor_rww_start_exclusive(nor));
	return ret;

If this wait is interrupted by a fatal signal, it returns -ERESTARTSYS
directly without calling spi_nor_unprep().

Can this permanently leak SPI controller resources like clocks and PM
runtime references?


[Severity: Critical]
Does using a killable wait here allow signals to abort the essential hardware
teardown?

If the process triggering the unbind receives a fatal signal, the lock
acquisition aborts and returns an error. This causes spi_nor_remove() to
skip the essential spi_nor_restore() hardware reset step, leaving the
physical flash in a non-default active state (e.g. 4-byte addressing mode).

Can this cause the system to become unbootable on the next boot if the
bootloader fails to read the flash in the unexpected 4-byte addressing mode?

> +	if (!ret) {
> +		spi_nor_restore(nor);
> +		spi_nor_unlock_and_unprep(nor);
> +	}

[Severity: High]
This is a pre-existing issue, but does dropping the lock here re-open the
race window before the MTD device is unregistered?

Concurrent I/O operations can acquire the lock after
spi_nor_unlock_and_unprep() but before or during mtd_device_unregister()
(which can trigger block layer flushes via mtdblock_release()).

These operations will execute using the driver's cached software state
(4-byte addressing), but the physical chip has already been restored to
3-byte addressing.

Can this result in silent data corruption?

>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);
>  }

[Severity: Critical]
This is a pre-existing issue, but is it possible for devm-allocated driver
data to be freed while userspace can still access the MTD device?

In drivers/mtd/spi-nor/core.c:spi_nor_probe(), struct spi_nor is allocated
using devm_kzalloc().

During spi_nor_remove(), mtd_device_unregister() drops a reference to the
MTD device, but does not block if userspace still holds an open file
descriptor:

	return mtd_device_unregister(&nor->mtd);

Since spi_nor_remove() returns immediately, the driver core will execute
devres cleanup, freeing struct spi_nor.

Could subsequent MTD operations from the open file descriptor dereference
the freed memory?

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