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

[PATCH 0/2] 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 fixes ->shutdown, which every reboot and kexec goes through,
and is marked for stable. Patch 2 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.

Note what patch 1 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.

The patches are independent; patch 2 can be dropped without affecting
patch 1.

Itai Handler (2):
  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 | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

[PATCH 1/2] 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 ccf4396cdcd0..96dd6ae6d619 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);
 }
 
 /*

[PATCH 2/2] 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 96dd6ae6d619..51128c94d1ce 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);

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

[sashiko-bot]

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

New issues:
- [High] Unsafe Killable Wait in Shutdown/Remove skips hardware restoration

Pre-existing issues:
- [Critical] Race condition leading to data corruption after shutdown restore
- [High] Resource leak in locking helpers on fatal signal interruption
--

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

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

This commit modifies spi_nor_shutdown() to acquire the device lock before
restoring the flash addressing mode. This serializes the restore operation
against operations already in flight during system shutdown.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ccf4396cdcd04..96dd6ae6d6191 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);

[Severity: High]
This is a pre-existing issue, but does spi_nor_prep_and_lock() leak device
preparation resources if interrupted by a fatal signal?

If a process is killed while waiting on wait_event_killable() inside
spi_nor_prep_and_lock(), it returns -ERESTARTSYS but fails to invoke
spi_nor_unprep() to roll back the earlier spi_nor_prep() call. Does this
also apply to the other locking helpers like spi_nor_prep_and_lock_pe() and
spi_nor_prep_and_lock_rd()?

> +	if (ret)
> +		return;

[Severity: High]
If a fatal signal is pending during the reboot process, such as from the OOM
killer or a manual SIGKILL, wait_event_killable() in spi_nor_prep_and_lock()
will immediately return -ERESTARTSYS.

Will this early return skip the spi_nor_restore() hardware state reset? Since
this skips restoring the 3-byte addressing mode, could the bootloader read
garbage on the next boot, leaving the device unbootable until a power cycle?

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

[Severity: Critical]
This is a pre-existing issue, but could releasing the lock while the MTD
device remains active expose the flash's modified 3-byte addressing state
to concurrent operations?

If a concurrent thread acquires the lock immediately after spi_nor_shutdown()
finishes and issues a write command, the driver will assume 4-byte mode
and send 4 address bytes. Could the hardware misinterpret the 4th address
byte as data, resulting in permanent data corruption by shifting the
destination address?

>  }

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Releasing the lock before unregistering the MTD device allows queued MTD operations to execute on a flash that has been restored to 3-byte mode, corrupting writes and erases.
- [Critical] Unregistering the MTD device does not wait for existing users to close their file descriptors, leading to a Use-After-Free of `struct spi_nor` on subsequent I/O.
- [High] Missing `spi_nor_unprep()` in the error path of `spi_nor_prep_and_lock()` causes permanent hardware resource leaks and deadlocks.
--

commit cca0f80dbf08e34d8606b95f2df252dd1ddf153c
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 commit fixes
the issue by taking nor->lock for the restore to prevent unbinds
from running the restore while another thread is in the middle of
an operation.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 96dd6ae6d6191..51128c94d1ce3 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 spi_nor_prep_and_lock() leak resources
if interrupted by a signal?

If a user issues an operation using parallel locking (SNOR_F_RWW),
spi_nor_prep_and_lock() acquires resources via spi_nor_prep() and then enters
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))
        ...
    else
        ret = wait_event_killable(nor->rww.wait,
                                  spi_nor_rww_start_exclusive(nor));

    return ret;
}

If a signal interrupts the wait, wait_event_killable() returns -ERESTARTSYS.
The function returns this error immediately without calling spi_nor_unprep(),
permanently leaking the acquired controller resources (such as clocks or
power management runtime) and deadlocking the mutex.

> +	if (!ret) {
> +		spi_nor_restore(nor);
> +		spi_nor_unlock_and_unprep(nor);
> +	}
>  
>  	/* Clean up MTD stuff. */
>  	return mtd_device_unregister(&nor->mtd);

[Severity: Critical]
This is a pre-existing issue, but does dropping the lock before unregistering
the MTD device leave a race window where pending operations can wake up and
execute?

A userspace process can queue operations (e.g., via a blocked character
device) while the SPI NOR driver is concurrently unbound. Since the lock is
released here, and mtd_device_unregister() doesn't wait for pending
operations to finish, the queued operations can acquire the lock and execute
before the device is fully torn down.

If this happens, the operations will execute while the driver software expects
4-byte addressing but the hardware has been restored to 3-byte mode, leading
to corrupted reads and writes.

[Severity: Critical]
This is a pre-existing issue, but does unregistering the MTD device without
waiting for existing users to close their file descriptors lead to a
Use-After-Free of struct spi_nor?

A userspace process can open the MTD device, incrementing its kref. When the
driver is unbound, spi_nor_remove() calls mtd_device_unregister().

As noted in the commit message, del_mtd_device() merely drops a reference
without waiting for active users to close the device.

When spi_nor_remove() returns, devres will immediately free the struct spi_nor
memory (which was allocated via devm_kzalloc), but the userspace process may
still issue operations or close the file descriptor, dereferencing the freed
memory.

>  }

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

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

[Itai Handler]

Thanks for the review. Taking the three points in turn: the first is
unreachable, but checking it turned up a real bug next to it that this
series has to fix first; the second cannot arise on the path this patch
serves; and the third is correct and is already stated in the commit
message.

> [Severity: High]
> This is a pre-existing issue, but does spi_nor_prep_and_lock() leak device
> preparation resources if interrupted by a fatal signal?
>
> If a process is killed while waiting on wait_event_killable() inside
> spi_nor_prep_and_lock(), it returns -ERESTARTSYS but fails to invoke
> spi_nor_unprep() to roll back the earlier spi_nor_prep() call.

The asymmetry in the code is real, but the leak is not: the two halves
are mutually exclusive, so there is never anything to roll back.

spi_nor_prep() only does something when the flash has controller_ops:

        static int spi_nor_prep(struct spi_nor *nor)
        {
                int ret = 0;

                if (nor->controller_ops && nor->controller_ops->prepare)
                        ret = nor->controller_ops->prepare(nor);

                return ret;
        }

and the wait_event_killable() is reached only on the parallel-locking
branch, i.e. only when SNOR_F_RWW is set. That flag is set in
spi_nor_late_init_params() (core.c:2981):

        if (flags & SPI_NOR_RWW && nor->params->n_banks > 1 &&
            !nor->controller_ops)
                nor->flags |= SNOR_F_RWW;

Note the !nor->controller_ops. A flash that can reach the killable wait
by definition has no controller_ops, so spi_nor_prep() was a no-op and
spi_nor_unprep() would be one too. RWW and controller_ops cannot coexist.

> Does this
> also apply to the other locking helpers like spi_nor_prep_and_lock_pe() and
> spi_nor_prep_and_lock_rd()?

Those two have the same shape and so the same answer: same
spi_nor_prep(), same wait_event_killable() on the same SNOR_F_RWW
branch, so there is nothing to unprep there either.

One correction as well: on that branch spi_nor_prep_and_lock() does not
take nor->lock at all, so the error return cannot deadlock the mutex in
the way described.

That said, the neighbouring code is not fine, and I would not have
looked without this comment.

spi_nor_rww_start_exclusive() returns with nor->lock held, on both
paths:

        static bool spi_nor_rww_start_exclusive(struct spi_nor *nor)
        {
                struct spi_nor_rww *rww = &nor->rww;

                mutex_lock(&nor->lock);

                if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
                        return false;
                ...
                return true;
        }

It is the only one of the ten spi_nor_rww_{start,end}_* helpers that
still uses a bare mutex_lock(); the other nine all use
guard(mutex)(&nor->lock). Commit 03e7bb864d9a ("mtd: spi-nor: use
scope-based mutex cleanup helpers") replaced its "goto busy" with a
plain "return false" and deleted the busy: label that did the
mutex_unlock(), but did not add the guard() that the others got. Before
that commit the function always released the lock.

So on an RWW flash:

 - 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(), which does guard(mutex)(&nor->lock) and
   self-deadlocks on a non-recursive mutex;

 - if it is busy, it returns false with nor->lock held and
   wait_event_killable() then sleeps holding it, so nothing can ever
   clear ongoing_*.

That has been the case since v6.15. Nothing hits it 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. That is exactly what this patch
changes - spi_nor_shutdown() did not take the flash lock before, so an
RWW part never reached the exclusive path, and with my patch every
reboot would. v2 therefore leads with the guard(mutex) fix as patch 1/3
rather than building on top of it.

> [Severity: High]
> If a fatal signal is pending during the reboot process, such as from the OOM
> killer or a manual SIGKILL, wait_event_killable() in spi_nor_prep_and_lock()
> will immediately return -ERESTARTSYS.
>
> Will this early return skip the spi_nor_restore() hardware state reset?

Only on an RWW flash, for the same reason as above: the non-parallel
branch uses mutex_lock(), which is uninterruptible and cannot fail, and
spi_nor_prep() cannot fail on the spi-mem path because controller_ops is
NULL there. So on everything else spi_nor_prep_and_lock() returns 0.

Where it can happen, skipping the restore is exactly what the code does
today when the restore fails, and spi_nor_restore() is best-effort by
design - it ignores the error from spi_nor_set_4byte_addr_mode() and
only logs it, "in the hope that the flash will default to the 3-byte
address mode after the software reset". The patch does not make that
outcome more likely; the unlocked restore it replaces reports success
while leaving the flash in 4-byte mode, which is the failure being
fixed.

> [Severity: Critical]
> This is a pre-existing issue, but could releasing the lock while the MTD
> device remains active expose the flash's modified 3-byte addressing state
> to concurrent operations?

Yes, and this is the right thing to have flagged. It is in the commit
message:

        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.

The mechanism as described is right: a page program issued in that state
sends four address bytes to a chip expecting three, so the fourth is
consumed as the first data byte and the write lands shifted.

It is not introduced here, and it cannot be fixed here. The window
exists because ->shutdown runs with userspace still running and MTD
still accepting operations; the only real fix is at the MTD layer, and
it is a separate discussion I am happy to have. What this patch removes
is the strictly worse case where the restore is issued into a flash that
is mid-erase and silently discards it.

Thanks,
Itai

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

[Itai Handler]

Thanks for the review. All three points are marked pre-existing, and
that is the right call for all three: none of them is introduced by this
patch, and the last one is a genuine problem that this patch cannot
reach.

> [Severity: High]
> This is a pre-existing issue, but does spi_nor_prep_and_lock() leak resources
> if interrupted by a signal?
[...]
> The function returns this error immediately without calling spi_nor_unprep(),
> permanently leaking the acquired controller resources (such as clocks or
> power management runtime) and deadlocking the mutex.

Same answer as on 1/2, where this was raised as well: the asymmetry is
real but unreachable, because the two halves are mutually exclusive.

spi_nor_prep() does nothing unless nor->controller_ops->prepare exists,
and the wait_event_killable() is reached only when SNOR_F_RWW is set.
SNOR_F_RWW is set only when there are no controller_ops at all
(core.c:2981):

        if (flags & SPI_NOR_RWW && nor->params->n_banks > 1 &&
            !nor->controller_ops)
                nor->flags |= SNOR_F_RWW;

So a flash that can be interrupted in that wait never acquired anything
in spi_nor_prep(), and there are no clocks or runtime-PM references to
leak. The mutex is not taken on that branch either, so the error return
cannot deadlock it.

While checking this I did find a real bug next to it:
spi_nor_rww_start_exclusive() returns with nor->lock held on both paths,
because 03e7bb864d9a ("mtd: spi-nor: use scope-based mutex cleanup
helpers") removed its mutex_unlock() without adding the
guard(mutex)(&nor->lock) that the other nine helpers got. No caller
reaches it today, which is how it survived since v6.15, but this patch
is one of the two that would. It leads v2 as patch 1/3. Details are in
my reply to the review of 1/2.

> [Severity: Critical]
> This is a pre-existing issue, but does dropping the lock before unregistering
> the MTD device leave a race window where pending operations can wake up and
> execute?

Yes. This is the same residual that patch 1/2 documents in its commit
message for ->shutdown, and it has the same cause: the restore runs
while MTD is still accepting operations, so an operation that starts
after the lock is dropped will send four address bytes to a chip that is
now in 3-byte mode.

Serialising against the operations already in flight is what this patch
is for - it stops the restore itself being issued into a busy flash,
which is the case where the flash silently discards it and stays in
4-byte mode. Closing the remaining window needs MTD to stop accepting
operations before the driver is torn down, which is a layer above this
one and a separate discussion.

> [Severity: Critical]
> This is a pre-existing issue, but does unregistering the MTD device without
> waiting for existing users to close their file descriptors lead to a
> Use-After-Free of struct spi_nor?
[...]
> When spi_nor_remove() returns, devres will immediately free the struct spi_nor
> memory (which was allocated via devm_kzalloc), but the userspace process may
> still issue operations or close the file descriptor, dereferencing the freed
> memory.

This one has substance and I do not want to wave it away, but it is
independent of this patch: it is identical with the patch and without
it, since the patch only adds a lock around the restore and does not
touch the teardown order or any lifetime.

For what it is worth, the analysis looks right to me as far as it goes.
nor is devm_kzalloc()'d in spi_nor_probe() and nor->mtd is embedded in
it, so the allocation is owned by devres and is freed when the device is
unbound. The kref added by 19bfa9ebebb5 ("mtd: use refcount to prevent
corruption") governs when mtd_device_release() runs, not when that
backing memory goes away. __get_mtd_device() does try_module_get(), so
an open user pins the module, but a sysfs unbind is not a module unload
and is not blocked by it.

So the question is a real one, but it is an MTD-core and devres lifetime
question rather than a spi-nor locking one, and the answer cannot live
in spi_nor_remove(). Folding it into this series would mix two unrelated
changes. If it holds up under a closer look I will report it separately
with a reproducer rather than attach it here.

Thanks,
Itai

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

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 7188 bytes --]

On Mon Sep 14, 2026 at 10:08 AM CEST, Itai Handler wrote:
> Thanks for the review. Taking the three points in turn: the first is
> unreachable, but checking it turned up a real bug next to it that this
> series has to fix first; the second cannot arise on the path this patch
> serves; and the third is correct and is already stated in the commit
> message.

So is this now two AI agents chatting with each other?

Sorry, but please be more precise and on point and drop that wall of
text.

And please annotate your commits with the Assisted-by: tag.

-michael

>> [Severity: High]
>> This is a pre-existing issue, but does spi_nor_prep_and_lock() leak device
>> preparation resources if interrupted by a fatal signal?
>>
>> If a process is killed while waiting on wait_event_killable() inside
>> spi_nor_prep_and_lock(), it returns -ERESTARTSYS but fails to invoke
>> spi_nor_unprep() to roll back the earlier spi_nor_prep() call.
>
> The asymmetry in the code is real, but the leak is not: the two halves
> are mutually exclusive, so there is never anything to roll back.
>
> spi_nor_prep() only does something when the flash has controller_ops:
>
>         static int spi_nor_prep(struct spi_nor *nor)
>         {
>                 int ret = 0;
>
>                 if (nor->controller_ops && nor->controller_ops->prepare)
>                         ret = nor->controller_ops->prepare(nor);
>
>                 return ret;
>         }
>
> and the wait_event_killable() is reached only on the parallel-locking
> branch, i.e. only when SNOR_F_RWW is set. That flag is set in
> spi_nor_late_init_params() (core.c:2981):
>
>         if (flags & SPI_NOR_RWW && nor->params->n_banks > 1 &&
>             !nor->controller_ops)
>                 nor->flags |= SNOR_F_RWW;
>
> Note the !nor->controller_ops. A flash that can reach the killable wait
> by definition has no controller_ops, so spi_nor_prep() was a no-op and
> spi_nor_unprep() would be one too. RWW and controller_ops cannot coexist.
>
>> Does this
>> also apply to the other locking helpers like spi_nor_prep_and_lock_pe() and
>> spi_nor_prep_and_lock_rd()?
>
> Those two have the same shape and so the same answer: same
> spi_nor_prep(), same wait_event_killable() on the same SNOR_F_RWW
> branch, so there is nothing to unprep there either.
>
> One correction as well: on that branch spi_nor_prep_and_lock() does not
> take nor->lock at all, so the error return cannot deadlock the mutex in
> the way described.
>
> That said, the neighbouring code is not fine, and I would not have
> looked without this comment.
>
> spi_nor_rww_start_exclusive() returns with nor->lock held, on both
> paths:
>
>         static bool spi_nor_rww_start_exclusive(struct spi_nor *nor)
>         {
>                 struct spi_nor_rww *rww = &nor->rww;
>
>                 mutex_lock(&nor->lock);
>
>                 if (rww->ongoing_io || rww->ongoing_rd || rww->ongoing_pe)
>                         return false;
>                 ...
>                 return true;
>         }
>
> It is the only one of the ten spi_nor_rww_{start,end}_* helpers that
> still uses a bare mutex_lock(); the other nine all use
> guard(mutex)(&nor->lock). Commit 03e7bb864d9a ("mtd: spi-nor: use
> scope-based mutex cleanup helpers") replaced its "goto busy" with a
> plain "return false" and deleted the busy: label that did the
> mutex_unlock(), but did not add the guard() that the others got. Before
> that commit the function always released the lock.
>
> So on an RWW flash:
>
>  - 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(), which does guard(mutex)(&nor->lock) and
>    self-deadlocks on a non-recursive mutex;
>
>  - if it is busy, it returns false with nor->lock held and
>    wait_event_killable() then sleeps holding it, so nothing can ever
>    clear ongoing_*.
>
> That has been the case since v6.15. Nothing hits it 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. That is exactly what this patch
> changes - spi_nor_shutdown() did not take the flash lock before, so an
> RWW part never reached the exclusive path, and with my patch every
> reboot would. v2 therefore leads with the guard(mutex) fix as patch 1/3
> rather than building on top of it.
>
>> [Severity: High]
>> If a fatal signal is pending during the reboot process, such as from the OOM
>> killer or a manual SIGKILL, wait_event_killable() in spi_nor_prep_and_lock()
>> will immediately return -ERESTARTSYS.
>>
>> Will this early return skip the spi_nor_restore() hardware state reset?
>
> Only on an RWW flash, for the same reason as above: the non-parallel
> branch uses mutex_lock(), which is uninterruptible and cannot fail, and
> spi_nor_prep() cannot fail on the spi-mem path because controller_ops is
> NULL there. So on everything else spi_nor_prep_and_lock() returns 0.
>
> Where it can happen, skipping the restore is exactly what the code does
> today when the restore fails, and spi_nor_restore() is best-effort by
> design - it ignores the error from spi_nor_set_4byte_addr_mode() and
> only logs it, "in the hope that the flash will default to the 3-byte
> address mode after the software reset". The patch does not make that
> outcome more likely; the unlocked restore it replaces reports success
> while leaving the flash in 4-byte mode, which is the failure being
> fixed.
>
>> [Severity: Critical]
>> This is a pre-existing issue, but could releasing the lock while the MTD
>> device remains active expose the flash's modified 3-byte addressing state
>> to concurrent operations?
>
> Yes, and this is the right thing to have flagged. It is in the commit
> message:
>
>         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.
>
> The mechanism as described is right: a page program issued in that state
> sends four address bytes to a chip expecting three, so the fourth is
> consumed as the first data byte and the write lands shifted.
>
> It is not introduced here, and it cannot be fixed here. The window
> exists because ->shutdown runs with userspace still running and MTD
> still accepting operations; the only real fix is at the MTD layer, and
> it is a separate discussion I am happy to have. What this patch removes
> is the strictly worse case where the restore is issued into a flash that
> is mid-erase and silently discards it.
>
> Thanks,
> Itai


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

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

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]

On Thu Sep 10, 2026 at 8:44 PM CEST, Itai Handler wrote:
> 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.

Why are these two different patches? This should be folded into the
former one.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]