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