Re: [PATCH v2 1/6] iio: adc: ti-ads1298: fix the data read path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: adc: ti-ads1298: race in ads1298_rdata_complete() before complete()
- [High] iio: adc: ti-ads1298: race in ads1298_buffer_predisable() on scan teardown
- [High] iio: adc: ti-ads1298: use-after-free on timeout in ads1298_wait_rdata_idle()
--

commit f34a13120edd070609343b6d64ffb0e2efc8cfd5
Author: Stephan Costa Barros <stephanbrrs8@gmail.com>

iio: adc: ti-ads1298: fix the data read path

This commit separates the SPI transfer buffers onto dedicated cachelines,
tracks buffered mode and in-flight RDATA transfers with a waitqueue, handles
spi_async() and RDATA message errors, and reads single-shot data before
releasing direct mode.

> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index 06c6353aa1b2e..1d32b1f47f726 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c

[ ... ]

> @@ -185,10 +198,49 @@ static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
>  	return spi_sync_transfer(priv->spi, &xfer, 1);
>  }
>  
> +/* Whether no RDATA transfer is queued or in flight */
> +static bool ads1298_rdata_idle(struct ads1298_private *priv)
> +{
> +	guard(spinlock_irqsave)(&priv->irq_busy_lock);
> +	return !priv->rdata_xfer_busy;
> +}
> +
> +static int ads1298_wait_rdata_idle(struct ads1298_private *priv)
> +{
> +	if (!wait_event_timeout(priv->idle_wq, ads1298_rdata_idle(priv),
> +				msecs_to_jiffies(100)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +/* Called with irq_busy_lock held */
> +static void ads1298_rdata_start(struct ads1298_private *priv)
> +{
> +	int ret;
> +
> +	ret = spi_async(priv->spi, &priv->rdata_msg);
> +	if (!ret)
> +		return;
> +
> +	/* No completion will run for this transfer */
> +	priv->rdata_xfer_busy = 0;
> +	if (!priv->buffered) {
> +		priv->rdata_status = ret;
> +		complete(&priv->completion);
> +	}
> +	wake_up(&priv->idle_wq);
> +}
> +
>  static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
>  {
>  	int ret;
>  
> +	/* A transfer left over from an earlier timeout must finish first */
> +	ret = ads1298_wait_rdata_idle(priv);
> +	if (ret)
> +		return ret;
> +
>  	/* Enable the channel */
>  	ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(chan_index),
>  				 ADS1298_MASK_CH_PD, 0);

[ ... ]

> @@ -494,19 +544,32 @@ static void ads1298_rdata_complete(void *context)
>  {
>  	struct iio_dev *indio_dev = context;
>  	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int status = priv->rdata_msg.status;
>  	int scan_index;
>  	u32 *bounce = priv->bounce_buffer;
> +	bool buffered;
>  
> -	if (!iio_buffer_enabled(indio_dev)) {
> +	/* Only changes while no transfer is in flight */
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		buffered = priv->buffered;
> +
> +	if (!buffered) {
>  		/*
>  		 * for a single transfer mode we're kept in direct_mode until
>  		 * completion, avoiding a race with buffered IO.
>  		 */
> -		ads1298_rdata_unmark_busy(priv);
> +		priv->rdata_status = status;
> +		ads1298_rdata_release_busy_or_restart(priv);
>  		complete(&priv->completion);
>  		return;
>  	}

[Severity: High]
In ads1298_rdata_complete(), does calling
ads1298_rdata_release_busy_or_restart(priv) before complete(&priv->completion)
leave a race window when recovering from a timed-out single-shot read or
unbinding the driver?

Inside ads1298_rdata_release_busy_or_restart(priv), priv->rdata_xfer_busy is
set to 0, wake_up(&priv->idle_wq) is called, and priv->irq_busy_lock is
released before complete(&priv->completion) runs:

    ads1298_rdata_complete()
        if (!buffered) {
            priv->rdata_status = status;
            ads1298_rdata_release_busy_or_restart(priv)
                priv->rdata_xfer_busy = 0;
                wake_up(&priv->idle_wq);
            complete(&priv->completion);
        }

If an earlier call to ads1298_read_one() timed out at
wait_for_completion_timeout() while its spi_async() transfer was still in
flight, a subsequent ads1298_read_one() call waiting in
ads1298_wait_rdata_idle(priv) can wake up as soon as priv->rdata_xfer_busy
becomes 0. Because ads1298_regmap_config uses REGCACHE_MAPLE and neither
ADS1298_REG_CHnSET(chan_index) nor ADS1298_REG_CONFIG4 is volatile, repeating
a read on the same channel hits the regmap cache without doing any SPI bus
transfers and reaches reinit_completion(&priv->completion) immediately:

    ads1298_read_one()
        ads1298_wait_rdata_idle(priv)
        regmap_update_bits(..., ADS1298_REG_CHnSET(chan_index), ...)
        regmap_update_bits(..., ADS1298_REG_CONFIG4, ...)
        reinit_completion(&priv->completion);

If reinit_completion(&priv->completion) runs before the previous transfer's
ads1298_rdata_complete() executes complete(&priv->completion), that late
complete(&priv->completion) signals the newly reinitialized completion. The
second ads1298_read_one() then returns the old sample from priv->rx_buffer
while the new conversion and its SPI transfer are still in flight after direct
mode is released.

Similarly, if ads1298_quiesce() is waiting in ads1298_wait_rdata_idle(priv)
during driver unbind, it can return as soon as priv->rdata_xfer_busy is
cleared, allowing devres teardown to free indio_dev and priv before
complete(&priv->completion) finishes executing.

Should complete(&priv->completion) be called before releasing
priv->rdata_xfer_busy and waking priv->idle_wq (or while holding
priv->irq_busy_lock, matching ads1298_rdata_start())?

[ ... ]

> @@ -549,14 +615,39 @@ static int ads1298_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
>  
> -	return ads1298_write_cmd(priv, ADS1298_CMD_START);
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		priv->buffered = true;
> +
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_START);
> +	if (ret) {
> +		scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +			priv->buffered = false;
> +	}
> +
> +	return ret;
>  }
>  
>  static int ads1298_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int ret, ret2;
> +	int rate;
> +
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_STOP);
> +
> +	/*
> +	 * The conversion in progress completes and raises DRDY once more.
> +	 * Wait for it and for its transfer before the core tears down the
> +	 * scan.
> +	 */
> +	if (ads1298_get_samp_freq(priv, &rate) >= 0 && rate > 0)
> +		fsleep(2 * DIV_ROUND_UP(USEC_PER_SEC, rate));
> +	ret2 = ads1298_wait_rdata_idle(priv);
>  
> -	return ads1298_write_cmd(priv, ADS1298_CMD_STOP);
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		priv->buffered = false;
> +
> +	return ret ?: ret2;
>  }

[Severity: High]
Can an RDATA transfer still start or remain in flight when
ads1298_buffer_predisable() returns, racing with active_scan_mask teardown in
iio_disable_buffers()?

In drivers/iio/industrialio-buffer.c, iio_disable_buffers() frees and clears
indio_dev->active_scan_mask unconditionally after predisable() returns, even
when predisable() returns an error:

    iio_disable_buffers()
        if (indio_dev->setup_ops->predisable) {
            ret2 = indio_dev->setup_ops->predisable(indio_dev);
            if (ret2 && !ret)
                ret = ret2;
        }
        ...
        iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
        indio_dev->active_scan_mask = NULL;

In ads1298_buffer_predisable(), ads1298_interrupt() remains enabled and
unconditionally increments priv->rdata_xfer_busy and calls
ads1298_rdata_start(priv) whenever a falling edge occurs:

1. If ads1298_write_cmd(priv, ADS1298_CMD_STOP) fails (so the chip keeps
   converting), if ads1298_get_samp_freq(priv, &rate) fails (so fsleep() is
   skipped while the last conversion is still running), or if the final
   interrupt is delayed past the two-period fsleep(), ads1298_wait_rdata_idle()
   can observe priv->rdata_xfer_busy == 0 and return before the interrupt
   fires.

2. If the interrupt fires after ads1298_wait_rdata_idle(priv) drops
   priv->irq_busy_lock and before ads1298_buffer_predisable() acquires
   priv->irq_busy_lock to set priv->buffered = false, ads1298_interrupt()
   starts a new SPI transfer while priv->buffered is true, and
   ads1298_buffer_predisable() then sets priv->buffered = false while that
   transfer is in flight and returns without waiting for it.

3. While priv->buffered remains true during ads1298_wait_rdata_idle(priv),
   ads1298_rdata_release_busy_or_restart() continues starting new transfers if
   priv->rdata_xfer_busy > 1, and if ads1298_wait_rdata_idle(priv) returns
   -ETIMEDOUT, iio_disable_buffers() still frees indio_dev->active_scan_mask.

If ads1298_rdata_complete() reads priv->buffered == true, it iterates
iio_for_each_active_channel(indio_dev, scan_index) and dereferences
indio_dev->active_scan_mask while iio_disable_buffers() frees it and sets it
to NULL. If the interrupt or completion runs after priv->buffered = false is
set, ads1298_rdata_complete() takes the !buffered branch and overwrites
priv->rx_buffer and priv->rdata_status and completes priv->completion.

> @@ -569,6 +660,15 @@ static void ads1298_reg_disable(void *reg)
>  	regulator_disable(reg);
>  }
>  
> +static void ads1298_quiesce(void *data)
> +{
> +	struct ads1298_private *priv = data;
> +
> +	/* No new transfer may start, and the last one must end, before teardown */
> +	disable_irq(priv->spi->irq);
> +	ads1298_wait_rdata_idle(priv);
> +}

[Severity: High]
What happens in ads1298_quiesce() if the in-flight spi_async() transfer takes
longer than the 100ms timeout in ads1298_wait_rdata_idle()?

Once spi_async(priv->spi, &priv->rdata_msg) succeeds in ads1298_rdata_start(),
the SPI controller owns priv->rdata_msg, priv->rdata_xfer, priv->rx_buffer,
and priv->tx_buffer until the completion callback ads1298_rdata_complete()
runs. In drivers/spi/spi.c, spi_transfer_wait() uses a minimum transfer
timeout of 200ms (2 * ms + 200ms), and messages can also wait behind other
transfers on the SPI controller queue.

Because ads1298_quiesce() calls ads1298_wait_rdata_idle(priv) with a bounded
100ms wait_event_timeout() and ignores the return value, any stalled or
delayed SPI transfer that exceeds 100ms causes ads1298_quiesce() to return
while the transfer is still active. Devres teardown then frees indio_dev and
priv while the SPI controller is still using priv->rdata_msg and its buffers
and before ads1298_rdata_complete() is invoked on the freed context.

Should teardown use an uninterruptible wait_event() so that devres cannot free
priv before the SPI controller finishes with priv->rdata_msg?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024231335.280955-1-stephanbrrs8@gmail.com?part=1