Unverified Commit 40534d19 authored by Fabian Godehardt's avatar Fabian Godehardt Committed by Mark Brown
Browse files

spi: spidev: fix lock inversion between spi_lock and buf_lock 



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: default avatarFabian Godehardt <fg@emlix.com>
Link: https://patch.msgid.link/20260211072616.489522-1-fg@emlix.com


Signed-off-by: default avatarMark Brown <broonie@kernel.org>
parent 888a0a80

drivers/spi/spidev.c

+22 −41
Original line number Diff line number Diff line
@@ -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;
@@ -802,7 +784,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);