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

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 <[email protected]>
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

There's definitely a potential race there.

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.