Re: [PATCH v7 09/10] iio: pressure: dps310: implement .hwfifo_flush_to_buffer()

[Jonathan Cameron]

> Let userspace drain the FIFO on demand rather than only from the
> periodic drain.
> 
> With a trigger attached the FIFO is not running, so there is nothing to
> flush and the hook returns 0.
> 
> Assisted-by: LLM
> Signed-off-by: Rupesh Majhi <zoone.rupert@xxxxxxxxx>
Sashiko calls out a race condition in here on buffer disable.
We've been working on a few of these recently and subject to one
gap in buffer remove (hopefully fixed shortly) the simplest solution
seems to be claim_buffered_mode for regions that are accessing
the specific buffer mode, or the_active_scan_mask.

https://sashiko.dev/#/patchset/20260918122517.377565-1-zoone.rupert%40gmail.com

See the OSF sensor thread for more discussion on closing pretty much
the same race. It is present in a lot of drivers, so we will probably
be fixing this one for a while. 

Thanks,

Jonathan

>
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 024fcdc7f4e7..f888bff2c996 100644
> --- a/drivers/iio/pressure/dps310.c
> +++ b/drivers/iio/pressure/dps310.c


> @@ -1205,6 +1217,19 @@ static void dps310_fifo_hold_free(struct dps310_data *data)
>  	data->fifo_hold_max = 0;
>  }
>  
> +static int dps310_hwfifo_flush(struct iio_dev *iio, unsigned int count)
> +{
> +	struct dps310_data *data = iio_priv(iio);
> +
> +	/* A trigger drives the capture instead and leaves the FIFO empty */
> +	if (iio_device_get_current_mode(iio) != INDIO_BUFFER_SOFTWARE)
> +		return 0;

Sashiko: - this is the one I think we need to close.

[Severity: High]
If a user concurrently disables the buffer via sysfs, it calls
iio_disable_buffers(). This executes predisable() (which briefly takes
data->lock), frees indio_dev->active_scan_mask, and sets it to NULL.

Because the INDIO_BUFFER_SOFTWARE mode check is lockless, could
dps310_hwfifo_flush() wait for data->lock here, wake up after
predisable() finishes, and then call dps310_fifo_drain() which
dereferences the NULL pointer?

dps310_fifo_drain() will execute:

    bool pressure_enabled = test_bit(DPS310_SCAN_PRESSURE,
                                     data->iio->active_scan_mask);

Will this cause a NULL pointer dereference or use-after-free?
-

As talked abotu above, I think iio_device_claim_buffer_mode()
will do the job here as it stops the IIO core transitioning between
modes until afte this is done with the active_scan_mask.

> +
> +	guard(mutex)(&data->lock);
> +
> +	return dps310_fifo_drain(data, count);

Sashiko comment:
[Severity: Medium]
The hwfifo_flush_to_buffer IIO API contract specifies returning the number of
samples flushed, and a negative error ONLY if no samples were flushed.

If dps310_fifo_push_held() or dps310_fifo_push_scan() fails midway through
draining the FIFO in dps310_fifo_drain():

    rc = dps310_fifo_push_scan(data, data->fifo_temp_raw, raw[i]);
    if (rc)
        return rc;

won't returning the error code directly discard the successfully pushed sample
count (pushed), violating the IIO callback contract?

Returning a negative error when pushed > 0 causes iio_buffer_ready() to
swallow the error, return false, and force the reader thread to sleep instead
of reading the newly available samples.
-
In event of a hardware comms error I'm not sure we care that we drop
a few samples we might otherwise return. So I'd ignore this one.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>