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