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

[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

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 1/3] mtd: spi-nor: fix the lock left held by spi_nor_rww_start_exclusive()

[Miquel Raynal]

On 14/09/2026 at 11:11:47 +03, Itai Handler <itai.handler@gmail.com> wrote:

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

Michael, since we already got a cycle with AI walls of text just to tell
"fix that damn conversion", can I (or you) apply that other patch, so we
can get rid of these? Let me know what you prefer, although I don't
remember where that thread is.

Miquèl

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

[Michael Walle]

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

> Michael, since we already got a cycle with AI walls of text just to tell
> "fix that damn conversion", can I (or you) apply that other patch, so we
> can get rid of these? Let me know what you prefer, although I don't
> remember where that thread is.

It's already in spi-nor/next since last friday.

https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=spi-nor/next
https://lore.kernel.org/linux-mtd/178913311701.1915651.4791624215787396187.b4-ty@b4/T/#me3b51a242469a4d9722089e18547e87fdc65d7c9

-michael

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

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

[Miquel Raynal]

On 14/09/2026 at 14:20:05 +02, "Michael Walle" <mwalle@kernel.org> wrote:

>> Michael, since we already got a cycle with AI walls of text just to tell
>> "fix that damn conversion", can I (or you) apply that other patch, so we
>> can get rid of these? Let me know what you prefer, although I don't
>> remember where that thread is.
>
> It's already in spi-nor/next since last friday.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=spi-nor/next
> https://lore.kernel.org/linux-mtd/178913311701.1915651.4791624215787396187.b4-ty@b4/T/#me3b51a242469a4d9722089e18547e87fdc65d7c9
>
> -michael

Since that one is "big" I would have merged it through a fixes PR. I
usually send one around -rc4. So either you push the patch to mtd/fixes
(or I do it) and drop it from the spi-nor/next branch, or we keep things
like this, which is also kind of ok.

Regarding the use of `git push -f`, I know Richard, Pratyush & Tudor
highly dislike it and avoid to use it. I find this tool pretty useful on
branches like ours and prefer to drop a patch rather than carrying its
revert, or messing around with merges. I prefer a clean history rather
than the absolute lack of -f. We already have the (Linus) master branch
milestone which is set in stone, I don't get the need to create one
ourselves.

So, up to you ;)

Miquèl

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

[Michael Walle]

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

On Mon Sep 14, 2026 at 2:34 PM CEST, Miquel Raynal wrote:
> On 14/09/2026 at 14:20:05 +02, "Michael Walle" <mwalle@kernel.org> wrote:
>
>>> Michael, since we already got a cycle with AI walls of text just to tell
>>> "fix that damn conversion", can I (or you) apply that other patch, so we
>>> can get rid of these? Let me know what you prefer, although I don't
>>> remember where that thread is.
>>
>> It's already in spi-nor/next since last friday.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=spi-nor/next
>> https://lore.kernel.org/linux-mtd/178913311701.1915651.4791624215787396187.b4-ty@b4/T/#me3b51a242469a4d9722089e18547e87fdc65d7c9
>>
>> -michael
>
> Since that one is "big" I would have merged it through a fixes PR. I

A big one nobody cared until the rise of AI agents actively searching
for it? :)

> usually send one around -rc4. So either you push the patch to mtd/fixes
> (or I do it) and drop it from the spi-nor/next branch, or we keep things
> like this, which is also kind of ok.

I can move it into fixes though.

> Regarding the use of `git push -f`, I know Richard, Pratyush & Tudor
> highly dislike it and avoid to use it. I find this tool pretty useful on
> branches like ours and prefer to drop a patch rather than carrying its
> revert, or messing around with merges. I prefer a clean history rather
> than the absolute lack of -f. We already have the (Linus) master branch
> milestone which is set in stone, I don't get the need to create one
> ourselves.

Yeah, I'll probably use a force push as I prefer the clean history
too.

-michael

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

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

[Miquel Raynal]

On 14/09/2026 at 14:41:16 +02, "Michael Walle" <mwalle@kernel.org> wrote:

> On Mon Sep 14, 2026 at 2:34 PM CEST, Miquel Raynal wrote:
>> On 14/09/2026 at 14:20:05 +02, "Michael Walle" <mwalle@kernel.org> wrote:
>>
>>>> Michael, since we already got a cycle with AI walls of text just to tell
>>>> "fix that damn conversion", can I (or you) apply that other patch, so we
>>>> can get rid of these? Let me know what you prefer, although I don't
>>>> remember where that thread is.
>>>
>>> It's already in spi-nor/next since last friday.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=spi-nor/next
>>> https://lore.kernel.org/linux-mtd/178913311701.1915651.4791624215787396187.b4-ty@b4/T/#me3b51a242469a4d9722089e18547e87fdc65d7c9
>>>
>>> -michael
>>
>> Since that one is "big" I would have merged it through a fixes PR. I
>
> A big one nobody cared until the rise of AI agents actively searching
> for it? :)

I used quotes ;) But yeah it is big in the sense that it is an obvious
breakage and we are going to get  more people proposing a fix for it.

I usually do a single Fixes PR per cycle, and what comes next is carried
through the main changes PR during the merge window. I will tag and send
the fixes PR in a few days.

Miquèl

[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

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

[Miquel Raynal]

On 14/09/2026 at 11:11:48 +03, Itai Handler <itai.handler@gmail.com> wrote:

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

6 paragraphs to tell "Accesses to the flash should be
serialized". Please reduce it.

> 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.
> +	 */

I'm fine with the diff, but please drop this comment.

Thanks,
Miquèl