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

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

[Itai Handler]

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

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

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

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

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

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

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

base-commit: 50d05c7c76c96b90462f24debacca971d2e86713
-- 
2.34.1

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

[sashiko-bot]

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

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

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

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

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

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

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

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

    return ret;
}

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

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

[ ... ]

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

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

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

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

>  }
>  
>  /*

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

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

[Michael Walle]

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

On Tue Sep 15, 2026 at 1:52 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] mtd: spi-nor: skipped flash restore on fatal signal breaks boot
> - [High] mtd: spi-nor: shutdown hang due to ongoing erase operation

Does Sashiko have some valid points here? 

-michael

> --
>
> commit c0578fac7581b63c39bfd99ac3a6f934256685d3
> Author: Itai Handler <itai.handler@gmail.com>
>
> mtd: spi-nor: take the flash lock around spi_nor_restore()
>
> This commit changes spi_nor_shutdown() and spi_nor_remove() to acquire
> nor->lock around spi_nor_restore(). It aims to prevent operations like
> reads from overlapping with the restore, which could leave the chip in
> 3-byte addressing mode.
>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index ccf4396cdcd04..c891197ffa5b3 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3849,11 +3849,26 @@ static int spi_nor_probe(struct spi_mem *spimem)
>>  				   data ? data->nr_parts : 0);
>>  }
>>  
>> +/*
>> + * Restore between operations, not during one. Removal and shutdown both run
>> + * with MTD users still attached: a busy flash silently ignores the commands
>> + * spi_nor_restore() sends, and a restore landing inside a read changes the
>> + * chip's address width under the transfer.
>> + */
>> +static void spi_nor_restore_locked(struct spi_nor *nor)
>> +{
>> +	if (spi_nor_prep_and_lock(nor))
>> +		return;
>
> [Severity: High]
> Does this code silently skip the flash restore if a fatal signal breaks the
> lock acquisition? 
>
> For example, if a privileged user runs rmmod while an erase or read operation
> is actively holding the RWW lock, and the rmmod process receives SIGKILL,
> spi_nor_prep_and_lock() (via wait_event_killable()) will return -ERESTARTSYS:
>
> drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
>     ...
>     if (!spi_nor_use_parallel_locking(nor))
>         mutex_lock(&nor->lock);
>     else
>         ret = wait_event_killable(nor->rww.wait,
>                                   spi_nor_rww_start_exclusive(nor));
>
>     return ret;
> }
>
> Bypassing the hardware teardown path here leaves the flash in 4-byte addressing
> mode. This might cause the bootloader to fail to read the kernel on the next
> reboot, soft-bricking the device until a power cycle.
>
>> +
>> +	spi_nor_restore(nor);
>> +	spi_nor_unlock_and_unprep(nor);
>> +}
>> +
>
> [ ... ]
>
>> @@ -3863,7 +3878,7 @@ static void spi_nor_shutdown(struct spi_mem *spimem)
>>  {
>>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>>  
>> -	spi_nor_restore(nor);
>> +	spi_nor_restore_locked(nor);
>
> [Severity: High]
> Could this hang the system indefinitely during shutdown if an erase operation
> is ongoing?
>
> Erase operations (like spi_nor_erase_multi_sectors) hold nor->lock
> continuously for their entire duration, looping over spi_nor_wait_till_ready()
> with cond_resched(). A large chip erase can take several minutes.
>
> Because device_shutdown() iterates devices synchronously without a timeout,
> this synchronous wait for nor->lock could hang the reboot process until the
> erase finishes, forcing the user to hard-reset the machine.

Also I'm not sure, what would be the actual expected behavior here.
Cancel the erase? Wait until the erase is completed? Pratyush,
Miquel, do you have any thoughts on that?

-michael

>
>>  }
>>  
>>  /*


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