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

[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);
 }
 
 /*

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 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 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 1/2] mtd: spi-nor: take the flash lock in spi_nor_shutdown()

[Itai Handler]

On Mon, Sep 14, 2026 at 11:31 AM Michael Walle <mwalle@kernel.org> wrote:

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

A fair hit. I do use an assistant, and I should have said so up front
rather than leaving you to guess. Answering the bot on-list at that
length was a waste of your time as well; sorry for the noise.

>
> 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.

Understood, and both are addressed in v3: a single patch, a much
shorter commit message, and an Assisted-by: tag. I will not reply to
the bot again.

Thanks,
Itai