[PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi

linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi
       [not found] <20230116140448.116034-1-sashal@kernel.org>
@ 2023-01-16 14:04 ` Sasha Levin
  2023-01-16 14:40   ` Mark Brown
  2023-01-16 14:04 ` [PATCH AUTOSEL 5.10 12/17] spi: spidev: remove debug messages that access spidev->spi without locking Sasha Levin
  1 sibling, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2023-01-16 14:04 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Bartosz Golaszewski, Mark Brown, Sasha Levin, linux-spi

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[ Upstream commit a720416d94634068951773cb9e9d6f1b73769e5b ]

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 mutex.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Link: https://lore.kernel.org/r/20230106100719.196243-1-brgl@bgdev.pl
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/spi/spidev.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 9c5ec99431d2..87c4d641cbd5 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -67,7 +67,7 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
 
 struct spidev_data {
 	dev_t			devt;
-	spinlock_t		spi_lock;
+	struct mutex		spi_lock;
 	struct spi_device	*spi;
 	struct list_head	device_entry;
 
@@ -94,9 +94,8 @@ spidev_sync(struct spidev_data *spidev, struct spi_message *message)
 	int status;
 	struct spi_device *spi;
 
-	spin_lock_irq(&spidev->spi_lock);
+	mutex_lock(&spidev->spi_lock);
 	spi = spidev->spi;
-	spin_unlock_irq(&spidev->spi_lock);
 
 	if (spi == NULL)
 		status = -ESHUTDOWN;
@@ -106,6 +105,7 @@ spidev_sync(struct spidev_data *spidev, struct spi_message *message)
 	if (status == 0)
 		status = message->actual_length;
 
+	mutex_unlock(&spidev->spi_lock);
 	return status;
 }
 
@@ -358,12 +358,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);
+	mutex_lock(&spidev->spi_lock);
 	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
-
-	if (spi == NULL)
+	if (spi == NULL) {
+		mutex_unlock(&spidev->spi_lock);
 		return -ESHUTDOWN;
+	}
 
 	/* use the buffer lock here for triple duty:
 	 *  - prevent I/O (from us) so calling spi_setup() is safe;
@@ -500,6 +500,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 	mutex_unlock(&spidev->buf_lock);
 	spi_dev_put(spi);
+	mutex_unlock(&spidev->spi_lock);
 	return retval;
 }
 
@@ -521,12 +522,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);
+	mutex_lock(&spidev->spi_lock);
 	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
-
-	if (spi == NULL)
+	if (spi == NULL) {
+		mutex_unlock(&spidev->spi_lock);
 		return -ESHUTDOWN;
+	}
 
 	/* SPI_IOC_MESSAGE needs the buffer locked "normally" */
 	mutex_lock(&spidev->buf_lock);
@@ -553,6 +554,7 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
 done:
 	mutex_unlock(&spidev->buf_lock);
 	spi_dev_put(spi);
+	mutex_unlock(&spidev->spi_lock);
 	return retval;
 }
 
@@ -631,10 +633,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);
+	mutex_lock(&spidev->spi_lock);
 	/* ... after we unbound from the underlying device? */
 	dofree = (spidev->spi == NULL);
-	spin_unlock_irq(&spidev->spi_lock);
+	mutex_unlock(&spidev->spi_lock);
 
 	/* last close? */
 	spidev->users--;
@@ -761,7 +763,7 @@ static int spidev_probe(struct spi_device *spi)
 
 	/* Initialize the driver data */
 	spidev->spi = spi;
-	spin_lock_init(&spidev->spi_lock);
+	mutex_init(&spidev->spi_lock);
 	mutex_init(&spidev->buf_lock);
 
 	INIT_LIST_HEAD(&spidev->device_entry);
@@ -806,9 +808,9 @@ static int 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);
+	mutex_lock(&spidev->spi_lock);
 	spidev->spi = NULL;
-	spin_unlock_irq(&spidev->spi_lock);
+	mutex_unlock(&spidev->spi_lock);
 
 	list_del(&spidev->device_entry);
 	device_destroy(spidev_class, spidev->devt);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi
  2023-01-16 14:04 ` [PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi Sasha Levin
@ 2023-01-16 14:40   ` Mark Brown
  2023-01-23  0:59     ` Sasha Levin
  0 siblings, 1 reply; 4+ messages in thread
From: Mark Brown @ 2023-01-16 14:40 UTC (permalink / raw)
  To: Sasha Levin; +Cc: linux-kernel, stable, Bartosz Golaszewski, linux-spi

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

On Mon, Jan 16, 2023 at 09:04:42AM -0500, Sasha Levin wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> [ Upstream commit a720416d94634068951773cb9e9d6f1b73769e5b ]
> 
> 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().

There are ongoing discussions of race conditions with this commit.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi
  2023-01-16 14:40   ` Mark Brown
@ 2023-01-23  0:59     ` Sasha Levin
  0 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2023-01-23  0:59 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-kernel, stable, Bartosz Golaszewski, linux-spi

On Mon, Jan 16, 2023 at 02:40:05PM +0000, Mark Brown wrote:
>On Mon, Jan 16, 2023 at 09:04:42AM -0500, Sasha Levin wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> [ Upstream commit a720416d94634068951773cb9e9d6f1b73769e5b ]
>>
>> 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().
>
>There are ongoing discussions of race conditions with this commit.

Okay, I've dropped it. Thanks!

-- 
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH AUTOSEL 5.10 12/17] spi: spidev: remove debug messages that access spidev->spi without locking
       [not found] <20230116140448.116034-1-sashal@kernel.org>
  2023-01-16 14:04 ` [PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi Sasha Levin
@ 2023-01-16 14:04 ` Sasha Levin
  1 sibling, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2023-01-16 14:04 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Bartosz Golaszewski, Mark Brown, Sasha Levin, linux-spi

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[ Upstream commit 6b35b173dbc1711f8d272e3f322d2ad697015919 ]

The two debug messages in spidev_open() dereference spidev->spi without
taking the lock and without checking if it's not null. This can lead to
a crash. Drop the messages as they're not needed - the user-space will
get informed about ENOMEM with the syscall return value.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Link: https://lore.kernel.org/r/20230106100719.196243-2-brgl@bgdev.pl
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/spi/spidev.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 87c4d641cbd5..05b0585d5ced 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -594,7 +594,6 @@ static int spidev_open(struct inode *inode, struct file *filp)
 	if (!spidev->tx_buffer) {
 		spidev->tx_buffer = kmalloc(bufsiz, GFP_KERNEL);
 		if (!spidev->tx_buffer) {
-			dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
 			status = -ENOMEM;
 			goto err_find_dev;
 		}
@@ -603,7 +602,6 @@ static int spidev_open(struct inode *inode, struct file *filp)
 	if (!spidev->rx_buffer) {
 		spidev->rx_buffer = kmalloc(bufsiz, GFP_KERNEL);
 		if (!spidev->rx_buffer) {
-			dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
 			status = -ENOMEM;
 			goto err_alloc_rx_buf;
 		}
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-01-23  0:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20230116140448.116034-1-sashal@kernel.org>
2023-01-16 14:04 ` [PATCH AUTOSEL 5.10 11/17] spi: spidev: fix a race condition when accessing spidev->spi Sasha Levin
2023-01-16 14:40   ` Mark Brown
2023-01-23  0:59     ` Sasha Levin
2023-01-16 14:04 ` [PATCH AUTOSEL 5.10 12/17] spi: spidev: remove debug messages that access spidev->spi without locking Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).