Re: Unsynchronized access to spi bus by mmc_rescan?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jul 21, 2016 at 05:40:15PM -0400, Rich Felker wrote:

> The spi master device has a "bus lock flag" that's used in lieu of a
> recursive mutex, but it seems to be used entirely incorrectly. The
> logic I'm seeing in spi.c is essentially:

I am entirely unclear why you believe there is a recursive mutex here,
that's not the purpose of the spi_bus_lock() API at all.

> 	if (some_task_has_spi_bus_locked)
> 		use_spi_master_without_locking();

> There seems to be no check that the task that's using the master
> without locking is the same one that locked it!

That's because there is no requirement that something that takes a bus
lock be executing from a single context.  It is just granting exclusive
access to the bus, the caller is free to do those accesses in any manner
it sees fit.

> Also, master->bus_lock_flag is modified completely unsynchonized from
> read access to master->bus_lock_flag. There is no locking of
> master->bus_lock_mutex or master->bus_lock_spinlock around the read
> accesses to master->bus_lock_flag, and in one place the write (of 0)
> to master->bus_lock_flag takes place without holding the spinlock.

The critical read access is in spi_async where we exclude users that
don't hold the lock which *is* spinlocked; we should probably hold the
spinlock on release but it's not so important since we know that we hold
the lock when we call it and the mutex ends up providing protection.
spi_sync() shouldn't be reading the flag in the first place as far as I
can tell, it should unconditionally have the flag clear as locked
callers have to use spi_sync_locked() but that will give us a fast path
that doesn't check for the bus lock which starts to reveal the
underlying issue.

This underlying issue is that we are trying to use one mutex for two
purposes, the existing mutex is mainly being used to serialize access to
the physical bus but the externally visible bus lock is there to ensure
that only one caller is able to queue new transfers which is not the
same thing at all.  It is completely irrelevant when we are pushing
messages to the bus if we are doing so with the bus lock held.

> The obvious fix would be using an actual recursive mutex to obtain
> recursive mutex semantics.

This would be broken since we need to be able to start transfers from
atomic context.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux