Re: spidev locking ("spi: spidev: fix possible NULL dereference")

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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

On Fri, Nov 13, 2015 at 04:13:44PM +0100, Vegard Nossum wrote:

> spidev_remove() potentially sets spidev->spi to NULL, and this happens
> with the spidev->spi_lock taken, but without the device_list_lock mutex,
> and spidev_release() only takes the device_list_lock mutex for the
> spidev->spi dereference above, so presumably it is possible for a new
> caller to set spidev->spi to NULL while spidev_release() is in progress.

> I think it would be great if somebody who knows the code could have a
> look into this to see if there is indeed a problem, and there could be
> some comments added to the code to explain

There's definitely a potential race there.

> 1) why it is (not?) okay to check spidev->spi without the spinlock there,

> 2) why the ->spi_lock is not taken inside the device_list_lock mutex in
> spidev_remove(),

> 3) what are the potential interactions between spidev_release() and
> spidev_remove(), if any,

It's not safe to use spidev->spi there without the lock.  We're safe
otherwise because in order to have dereferenced spi elsewhere we must of
neccesity have a users refcount so spidev can't go away.  This is all
fairly standard really, it comes back to the issue Laurent was raising
on the kernel summit discussion list about not having helpful
infrastructure for managing lifetime with respect to both device removal
and files.

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