Re: [PATCH v2 08/15] iio: adc: ad4134: Support buffered data read

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi Jonathan,

On 09/21, Jonathan Cameron wrote:
> > Enable users to run buffered data captures triggered by IIO trigger device.
> > Add an IIO timestamp channel so each data scan is provided with measurement
> > time information. Require single-read operations to be in IIO device direct
> > access mode to prevent buffered and single-shot data captures to disrupt
> > each other.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
> Hi Marcelo
> 
> Sashiko had a bunch of feedback on this one.  Note that you can save
> time if you check out what it comes up with and reply to the thread
> to say what is valid and not.

Yes, I've been working on fixes for the issues that seem legitimate. Most of
them do seem to make sense. I was to reply only to the ones that seem off, but
didn't yet manage to fully assess them all. Will start providing preliminary
reply to automated reviews.

> 
> I haven't yet asked for those emails to go directly to linux-iio
> as the quality is mixed and I am reluctant to confuse newcomers, but
> most like I will ask them to turn those emails on in a month or so
> if not before. In meantime more experienced folk can look at the
> website directly.
> 
> > diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> > index 30c7b1d1bc67..0b6843bf8a9e 100644
> 
> >  
> 
> >  
> >  struct ad4134_state {
> > @@ -149,7 +161,10 @@ struct ad4134_state {
> >  	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> >  	 * to live in their own cache lines.
> >  	 */
> > -	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> > +	union {
> > +		u8 reg[AD4134_SPI_MAX_XFER_LEN];
> > +		u32 scan[ARRAY_SIZE(ad4134_chan_set)];
> 
> 
> Sashiko:
> [Severity: High]
> Does this array size fail to account for the timestamp's alignment padding?
> 
> The array is fixed at 20 bytes (5 elements). When all 4 channels and the 64-bit
> timestamp are active, the IIO core computes scan_bytes as 24 bytes due to the
> timestamp's 8-byte alignment requirement (16 bytes data + 8 bytes padding and
> timestamp). Passing sizeof(st->rx_buf.scan) to iio_push_to_buffers_with_ts()
> later in the trigger handler will cause the IIO core's safety check to fail,
> aborting the buffer push with -ENOSPC on full-scan captures.
> -
> 
> That does indeed look wrong. I'd expect to see one of the DECLARE
> buffer macros used here to ensure a large enough buffer.  This
> should have shown up in testing so I'm curious why it didn't.

I had initially tested with only one or two channels enabled while requesting
timestamp. This issue is going to be fixed in v3.

> 
> [Severity: High]
> Can concurrent lockless accesses corrupt the rx_buf shared union?
> 
> The ad4134_trigger_handler() executes locklessly in a pollfunc context, while
> regmap debugfs reads/writes (which do not acquire the IIO direct mode lock) can
> concurrently use st->rx_buf.reg as a DMA buffer. This can result in corrupted
> SPI scan data, corrupted regmap read/write data, and potentially invalid
> hardware configurations or ADC samples if debugfs is accessed while a triggered
> buffer capture is running.
> 
> -
> Also sounds valid though I haven't confirmed everything stated.
> A buffer like this should be protected by a local lock, not rely on
> serialization via the mode claim mechanism.

Yes, that's a valid one that is going to be fixed.

> 
> > +	} rx_buf __aligned(IIO_DMA_MINALIGN);
> >  	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
> >  };
> 
> 
> > @@ -281,7 +296,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
> >  		 * Clock out data from all channels to avoid that.
> >  		 */
> >  		if (i == AD4134_VREG_CH(reg))
> > -			sample = get_unaligned_be24(st->rx_buf);
> > +			sample = get_unaligned_be24(st->rx_buf.reg);
> >  	}
> >  	*val = sign_extend32(sample, AD4134_CHAN_PRECISION_BITS - 1);
> 
> [Severity: Medium]
> Does this leak uninitialized stack memory if the soft timestamp channel is read
> as a virtual register?
> 
> Because ad4134_regmap_config sets max_register to include the soft timestamp
> channel (ARRAY_SIZE(ad4134_chan_set) includes the timestamp, making index 4
> valid), userspace could read virtual register index 4 via regmap debugfs. In
> that case, this loop iterates only up to AD4134_NUM_CHANNELS (indices 0 to 3),
> meaning `i == AD4134_VREG_CH(reg)` (where AD4134_VREG_CH(reg) would be 4) is
> never true. The sample variable is left uninitialized, and an uninitialized
> 32-bit stack chunk is processed and returned to userspace.
> -
> Please check this one.
> >  
> 
Ack

> 
> 
> > @@ -333,6 +348,37 @@ static const struct regmap_config ad4134_regmap_config = {
> >  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)),
> >  };
> >  
> > +static irqreturn_t ad4134_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct ad4134_state *st = iio_priv(indio_dev);
> > +	unsigned int i = 0;
> > +	int ret;
> > +
> > +	gpiod_set_value_cansleep(st->odr_gpio, 1);
> > +	fsleep(1);
> > +	gpiod_set_value_cansleep(st->odr_gpio, 0);
> > +
> > +	for (unsigned int ch = 0; ch < AD4134_NUM_CHANNELS; ch++) {
> > +		ret = spi_write_then_read(st->spi, NULL, 0, &st->rx_buf.scan[ch],
> > +					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
> > +		if (ret)
> > +			goto err_out;
> > +
> > +		if (test_bit(ch, indio_dev->active_scan_mask) && ch != i)
> > +			memcpy(&st->rx_buf.scan[i++], &st->rx_buf.scan[ch],
> 
> [Severity: High]
> Does this conditional logic permanently corrupt the scan data by overwriting
> active channels?
> 
> When the first active channel has the same index as the destination (for example,
> both are 0), the condition `ch != i` evaluates to false, short-circuiting the
> if statement and entirely skipping the post-increment `i++`. The next active
> channel will then overwrite the previous data by being written to scan[0] again.
> This triggers on any standard buffered capture involving multiple channels.
> -
> 
> Check this one as well. Looks plausible to me.

Ack.

Thanks,
Marcelo