diff mbox series

[1/2] spi: spidev: fix a race condition when accessing spidev->spi

Message ID 20230105124100.185542-1-brgl@bgdev.pl
State Superseded
Headers show
Series [1/2] spi: spidev: fix a race condition when accessing spidev->spi | expand

Commit Message

Bartosz Golaszewski Jan. 5, 2023, 12:40 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's a spinlock in place that is taken in file_operations callbacks
whenever we check if spidev->spi is still alive (not null). It's also
taken when spidev->spi is set to NULL in remove().

This however doesn't protect the code against driver unbind event while
one of the syscalls is still in progress. To that end we need a lock taken
continuously as long as we may still access spidev->spi. As both the file
ops and the remove callback are never called from interrupt context, we
can replace the spinlock with a sleeping lock. Using an RW semaphore
allows the syscalls to run concurrently unless protected otherwise. We
take it for writing only when setting spidev->spi to null, while
everywhere else it's only taken for reading. This assures that it will
be dropped only once all currently executed syscalls have returned.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/spi/spidev.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

Mark Brown Jan. 5, 2023, 1:52 p.m. UTC | #1 
On Thu, Jan 05, 2023 at 01:40:59PM +0100, Bartosz Golaszewski wrote:

> can replace the spinlock with a sleeping lock. Using an RW semaphore
> allows the syscalls to run concurrently unless protected otherwise. We

I'm not sure this is important or useful, there's not a lot that can
practically happen concurrently when we get to actually interacting with
the device and it's making the code a bit less clear.
Bartosz Golaszewski Jan. 5, 2023, 2:30 p.m. UTC | #2 
On Thu, Jan 5, 2023 at 2:52 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jan 05, 2023 at 01:40:59PM +0100, Bartosz Golaszewski wrote:
>
> > can replace the spinlock with a sleeping lock. Using an RW semaphore
> > allows the syscalls to run concurrently unless protected otherwise. We
>
> I'm not sure this is important or useful, there's not a lot that can
> practically happen concurrently when we get to actually interacting with
> the device and it's making the code a bit less clear.

You suggest to just use a mutex instead?

Bart
Mark Brown Jan. 5, 2023, 2:43 p.m. UTC | #3 
On Thu, Jan 05, 2023 at 03:30:47PM +0100, Bartosz Golaszewski wrote:
> On Thu, Jan 5, 2023 at 2:52 PM Mark Brown <broonie@kernel.org> wrote:

> > I'm not sure this is important or useful, there's not a lot that can
> > practically happen concurrently when we get to actually interacting with
> > the device and it's making the code a bit less clear.

> You suggest to just use a mutex instead?

Yeah.
diff mbox series

Patch

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 6313e7d0cdf8..b71620f64ec9 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -18,6 +18,7 @@ 
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/property.h>
+#include <linux/rwsem.h>
 #include <linux/slab.h>
 #include <linux/compat.h>
 
@@ -68,7 +69,7 @@  static_assert(N_SPI_MINORS > 0 && N_SPI_MINORS <= 256);
 
 struct spidev_data {
 	dev_t			devt;
-	spinlock_t		spi_lock;
+	struct rw_semaphore	sem;
 	struct spi_device	*spi;
 	struct list_head	device_entry;
 
@@ -95,9 +96,8 @@  spidev_sync(struct spidev_data *spidev, struct spi_message *message)
 	int status;
 	struct spi_device *spi;
 
-	spin_lock_irq(&spidev->spi_lock);
+	down_read(&spidev->sem);
 	spi = spidev->spi;
-	spin_unlock_irq(&spidev->spi_lock);
 
 	if (spi == NULL)
 		status = -ESHUTDOWN;
@@ -107,6 +107,7 @@  spidev_sync(struct spidev_data *spidev, struct spi_message *message)
 	if (status == 0)
 		status = message->actual_length;
 
+	up_read(&spidev->sem);
 	return status;
 }
 
@@ -359,12 +360,12 @@  spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	 * we issue this ioctl.
 	 */
 	spidev = filp->private_data;
-	spin_lock_irq(&spidev->spi_lock);
+	down_read(&spidev->sem);
 	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
-
-	if (spi == NULL)
+	if (spi == NULL) {
+		up_read(&spidev->sem);
 		return -ESHUTDOWN;
+	}
 
 	/* use the buffer lock here for triple duty:
 	 *  - prevent I/O (from us) so calling spi_setup() is safe;
@@ -508,6 +509,7 @@  spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 	mutex_unlock(&spidev->buf_lock);
 	spi_dev_put(spi);
+	up_read(&spidev->sem);
 	return retval;
 }
 
@@ -529,12 +531,12 @@  spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
 	 * we issue this ioctl.
 	 */
 	spidev = filp->private_data;
-	spin_lock_irq(&spidev->spi_lock);
+	down_read(&spidev->sem);
 	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
-
-	if (spi == NULL)
+	if (spi == NULL) {
+		up_read(&spidev->sem);
 		return -ESHUTDOWN;
+	}
 
 	/* SPI_IOC_MESSAGE needs the buffer locked "normally" */
 	mutex_lock(&spidev->buf_lock);
@@ -561,6 +563,7 @@  spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
 done:
 	mutex_unlock(&spidev->buf_lock);
 	spi_dev_put(spi);
+	up_read(&spidev->sem);
 	return retval;
 }
 
@@ -640,10 +643,10 @@  static int spidev_release(struct inode *inode, struct file *filp)
 	spidev = filp->private_data;
 	filp->private_data = NULL;
 
-	spin_lock_irq(&spidev->spi_lock);
+	down_read(&spidev->sem);
 	/* ... after we unbound from the underlying device? */
 	dofree = (spidev->spi == NULL);
-	spin_unlock_irq(&spidev->spi_lock);
+	up_read(&spidev->sem);
 
 	/* last close? */
 	spidev->users--;
@@ -776,7 +779,7 @@  static int spidev_probe(struct spi_device *spi)
 
 	/* Initialize the driver data */
 	spidev->spi = spi;
-	spin_lock_init(&spidev->spi_lock);
+	init_rwsem(&spidev->sem);
 	mutex_init(&spidev->buf_lock);
 
 	INIT_LIST_HEAD(&spidev->device_entry);
@@ -821,9 +824,9 @@  static void spidev_remove(struct spi_device *spi)
 	/* prevent new opens */
 	mutex_lock(&device_list_lock);
 	/* make sure ops on existing fds can abort cleanly */
-	spin_lock_irq(&spidev->spi_lock);
+	down_write(&spidev->sem);
 	spidev->spi = NULL;
-	spin_unlock_irq(&spidev->spi_lock);
+	up_write(&spidev->sem);
 
 	list_del(&spidev->device_entry);
 	device_destroy(spidev_class, spidev->devt);