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

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

[Vegard Nossum]

Hi,

I was reviewing some stable commits and came across this, which might
not be wrong, but looks weird in any case:

commit a56166134ad739e7448332e2af2aac59a0b6a385
Author: Sudip Mukherjee <sudipm.mukherjee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date:   Thu Sep 10 16:48:13 2015 +0530

     spi: spidev: fix possible NULL dereference

The "weird" part is this:

-               spidev->speed_hz = spidev->spi->max_speed_hz;
+               if (spidev->spi)
+                       spidev->speed_hz = spidev->spi->max_speed_hz;

                 /* ... after we unbound from the underlying device? */
                 spin_lock_irq(&spidev->spi_lock);
                 dofree = (spidev->spi == NULL);
                 spin_unlock_irq(&spidev->spi_lock);

It would seem strange that the second spidev->spi read requires a
spinlock, but the first one doesn't.

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

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,

to make it easier to review the code in the future.

Thanks,


Vegard
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

[-- 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 --]