| From 54ac41358d87fa31fe231ee357399688ae6461bc Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Wed, 11 Feb 2026 08:26:16 +0100 |
| Subject: spi: spidev: fix lock inversion between spi_lock and buf_lock |
| |
| From: Fabian Godehardt <fg@emlix.com> |
| |
| [ Upstream commit 40534d19ed2afb880ecf202dab26a8e7a5808d16 ] |
| |
| The spidev driver previously used two mutexes, spi_lock and buf_lock, |
| but acquired them in different orders depending on the code path: |
| |
| write()/read(): buf_lock -> spi_lock |
| ioctl(): spi_lock -> buf_lock |
| |
| This AB-BA locking pattern triggers lockdep warnings and can |
| cause real deadlocks: |
| |
| WARNING: possible circular locking dependency detected |
| spidev_ioctl() -> mutex_lock(&spidev->buf_lock) |
| spidev_sync_write() -> mutex_lock(&spidev->spi_lock) |
| *** DEADLOCK *** |
| |
| The issue is reproducible with a simple userspace program that |
| performs write() and SPI_IOC_WR_MAX_SPEED_HZ ioctl() calls from |
| separate threads on the same spidev file descriptor. |
| |
| Fix this by simplifying the locking model and removing the lock |
| inversion entirely. spidev_sync() no longer performs any locking, |
| and all callers serialize access using spi_lock. |
| |
| buf_lock is removed since its functionality is fully covered by |
| spi_lock, eliminating the possibility of lock ordering issues. |
| |
| This removes the lock inversion and prevents deadlocks without |
| changing userspace ABI or behaviour. |
| |
| Signed-off-by: Fabian Godehardt <fg@emlix.com> |
| Link: https://patch.msgid.link/20260211072616.489522-1-fg@emlix.com |
| Signed-off-by: Mark Brown <broonie@kernel.org> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/spi/spidev.c | 63 ++++++++++++++++---------------------------- |
| 1 file changed, 22 insertions(+), 41 deletions(-) |
| |
| diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c |
| index 653f82984216c..661b84f3ee622 100644 |
| --- a/drivers/spi/spidev.c |
| +++ b/drivers/spi/spidev.c |
| @@ -74,7 +74,6 @@ struct spidev_data { |
| struct list_head device_entry; |
| |
| /* TX/RX buffers are NULL unless this device is open (users > 0) */ |
| - struct mutex buf_lock; |
| unsigned users; |
| u8 *tx_buffer; |
| u8 *rx_buffer; |
| @@ -102,24 +101,6 @@ spidev_sync_unlocked(struct spi_device *spi, struct spi_message *message) |
| return status; |
| } |
| |
| -static ssize_t |
| -spidev_sync(struct spidev_data *spidev, struct spi_message *message) |
| -{ |
| - ssize_t status; |
| - struct spi_device *spi; |
| - |
| - mutex_lock(&spidev->spi_lock); |
| - spi = spidev->spi; |
| - |
| - if (spi == NULL) |
| - status = -ESHUTDOWN; |
| - else |
| - status = spidev_sync_unlocked(spi, message); |
| - |
| - mutex_unlock(&spidev->spi_lock); |
| - return status; |
| -} |
| - |
| static inline ssize_t |
| spidev_sync_write(struct spidev_data *spidev, size_t len) |
| { |
| @@ -132,7 +113,8 @@ spidev_sync_write(struct spidev_data *spidev, size_t len) |
| |
| spi_message_init(&m); |
| spi_message_add_tail(&t, &m); |
| - return spidev_sync(spidev, &m); |
| + |
| + return spidev_sync_unlocked(spidev->spi, &m); |
| } |
| |
| static inline ssize_t |
| @@ -147,7 +129,8 @@ spidev_sync_read(struct spidev_data *spidev, size_t len) |
| |
| spi_message_init(&m); |
| spi_message_add_tail(&t, &m); |
| - return spidev_sync(spidev, &m); |
| + |
| + return spidev_sync_unlocked(spidev->spi, &m); |
| } |
| |
| /*-------------------------------------------------------------------------*/ |
| @@ -157,7 +140,7 @@ static ssize_t |
| spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos) |
| { |
| struct spidev_data *spidev; |
| - ssize_t status; |
| + ssize_t status = -ESHUTDOWN; |
| |
| /* chipselect only toggles at start or end of operation */ |
| if (count > bufsiz) |
| @@ -165,7 +148,11 @@ spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos) |
| |
| spidev = filp->private_data; |
| |
| - mutex_lock(&spidev->buf_lock); |
| + mutex_lock(&spidev->spi_lock); |
| + |
| + if (spidev->spi == NULL) |
| + goto err_spi_removed; |
| + |
| status = spidev_sync_read(spidev, count); |
| if (status > 0) { |
| unsigned long missing; |
| @@ -176,7 +163,9 @@ spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos) |
| else |
| status = status - missing; |
| } |
| - mutex_unlock(&spidev->buf_lock); |
| + |
| +err_spi_removed: |
| + mutex_unlock(&spidev->spi_lock); |
| |
| return status; |
| } |
| @@ -187,7 +176,7 @@ spidev_write(struct file *filp, const char __user *buf, |
| size_t count, loff_t *f_pos) |
| { |
| struct spidev_data *spidev; |
| - ssize_t status; |
| + ssize_t status = -ESHUTDOWN; |
| unsigned long missing; |
| |
| /* chipselect only toggles at start or end of operation */ |
| @@ -196,13 +185,19 @@ spidev_write(struct file *filp, const char __user *buf, |
| |
| spidev = filp->private_data; |
| |
| - mutex_lock(&spidev->buf_lock); |
| + mutex_lock(&spidev->spi_lock); |
| + |
| + if (spidev->spi == NULL) |
| + goto err_spi_removed; |
| + |
| missing = copy_from_user(spidev->tx_buffer, buf, count); |
| if (missing == 0) |
| status = spidev_sync_write(spidev, count); |
| else |
| status = -EFAULT; |
| - mutex_unlock(&spidev->buf_lock); |
| + |
| +err_spi_removed: |
| + mutex_unlock(&spidev->spi_lock); |
| |
| return status; |
| } |
| @@ -379,14 +374,6 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) |
| |
| ctlr = spi->controller; |
| |
| - /* use the buffer lock here for triple duty: |
| - * - prevent I/O (from us) so calling spi_setup() is safe; |
| - * - prevent concurrent SPI_IOC_WR_* from morphing |
| - * data fields while SPI_IOC_RD_* reads them; |
| - * - SPI_IOC_MESSAGE needs the buffer locked "normally". |
| - */ |
| - mutex_lock(&spidev->buf_lock); |
| - |
| switch (cmd) { |
| /* read requests */ |
| case SPI_IOC_RD_MODE: |
| @@ -510,7 +497,6 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) |
| break; |
| } |
| |
| - mutex_unlock(&spidev->buf_lock); |
| spi_dev_put(spi); |
| mutex_unlock(&spidev->spi_lock); |
| return retval; |
| @@ -541,9 +527,6 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd, |
| return -ESHUTDOWN; |
| } |
| |
| - /* SPI_IOC_MESSAGE needs the buffer locked "normally" */ |
| - mutex_lock(&spidev->buf_lock); |
| - |
| /* Check message and copy into scratch area */ |
| ioc = spidev_get_ioc_message(cmd, u_ioc, &n_ioc); |
| if (IS_ERR(ioc)) { |
| @@ -564,7 +547,6 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd, |
| kfree(ioc); |
| |
| done: |
| - mutex_unlock(&spidev->buf_lock); |
| spi_dev_put(spi); |
| mutex_unlock(&spidev->spi_lock); |
| return retval; |
| @@ -790,7 +772,6 @@ static int spidev_probe(struct spi_device *spi) |
| /* Initialize the driver data */ |
| spidev->spi = spi; |
| mutex_init(&spidev->spi_lock); |
| - mutex_init(&spidev->buf_lock); |
| |
| INIT_LIST_HEAD(&spidev->device_entry); |
| |
| -- |
| 2.51.0 |
| |