Re: [PATCH v4 0/8] IIO: Fused set 1 and 2 of timestamp alignment fixes

From: Alexandru Ardelean <ardeleanalex@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio <linux-iio@vger.kernel.org>,
	Alexandru Ardelean <alexandru.ardelean@analog.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v4 0/8] IIO: Fused set 1 and 2 of timestamp alignment fixes
Date: Sun, 29 Nov 2020 18:15:28 +0200	[thread overview]
Message-ID: <CA+U=DsqUy8nvqmMYHBvU0CBOVAmEmcdqy-XYxpj7zTjqNQt4+Q@mail.gmail.com> (raw)
In-Reply-To: <20201129132225.08a81004@archlinux>

On Sun, Nov 29, 2020 at 3:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Dear All,
>
> Whilst I'm reasonably confident this series is correct (famous last words)
> I don't like applying anything non trivial without having had at least one
> set of additional eyes on it.
>
> As such, if anyone has a chance to do a quick sanity check that would be
> much appreciated!

This looks good AFAICT.
But it also looks like it could do with some re-design/re-thinking.
Maybe somehow moving more of this buffer + timestamp management
somewhere in IIO core.

It would change the paradigm a bit, in the sense that a driver would
ask IIO core for a buffer/scratchpad area, where to put the data read
from the device.
This buffer pool management could be interesting [in the long run].
Maybe with some zero-copy mechanism.

>
> Thanks
>
> Jonathan
>
> +CC a few additional helpful souls :)
>
> On Sun, 20 Sep 2020 12:27:34 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Took me a while to get back to these.  We have 2 new patches in here to
> > fix issues unrelated to the main topic, but which effect the buffer lengths.
> > I've done those as precursors so it is clear what is going on.
> >
> > Note there are still a few outstanding drivers to be fixed before we can
> > think about adding a warning if unaligned buffers are provided. Naturally
> > they are the hardest ones, or the ones where I couldn't work out how
> > the code is working today, so may take a little while.
> >
> > Changes since v3:
> > * Applied all the ones where only minor comment changes were needed.
> > * rpr0521: Fixed typo. Also added to patch description Mikko's information
> >   on why it would be costly to split off the interrupt read.
> > * st_uvis: Drop the pointless masking.
> > * mag3110: Rename element to temperature
> > * bmi160: Add fix to length of buffer.
> > * bmi160: Improve comments and carry forwards shorter length.
> > * mpl3115: Sufficiently unusual to need a 'special' comment and another review.
> > * ti-ads124s08: Add fix to length of buffer.
> > * ti-ads124s08: Expand comment to express the buffer length not all needed if
> >   not all channels are enabled.
> >
> > Changes since v2:
> > * bmc150-accel: Use sizeof() for channel size (Andy Shevchenko)
> > * st_uvis25: Use local variable for regmap call (Andy Shevchenko)
> > * st_lsm6dsx: Use array of scan[] rather than 3 structures (Lorenzo Bianconi)
> > * inv_mpu6050: Add patch switching to a regmap_noinc_read (Jean-Baptiste Maneyrol)
> > * ina2xx: Use a structure (previously failed to notice that works here)
> > * I've added clarifying notes to patch descriptions based on questions asked.
> >   These were mainly about why we didn't use the structure approach everywhere
> >   and why I've forced alignment in places it wasn't strictly needed.
> >
> > Previous cover letter:
> > A few notes based on questions on v1.
> >
> > 1. Why not use put_unaligned to avoid the whole thing?
> >    This interface can pass directly to a variety of in kernel users
> >    who would reasonably assume that we have true natural alignment.
> >    When it gets passed directly is subtle as it depends on whether
> >    the demux has to realign the data or not.  So enabling an extra
> >    channel could result in a previously working alignment no longer
> >    being true.
> >
> >    Even if this is fine for existing usecases we are likely to
> >    store up subtle long term issues if we don't fix it explicitly.
> >    It's also worth noting that the data channel sometimes suffered
> >    the same problem as the timestamp.
> >
> > 2. Why not specify explicit padding?
> >    In my view this is error prone in comparisom with relying on
> >    c to do the hard work for us.
> >
> > 3. Why not move the timestamp to the start?
> >    ABI breakage and as timestamp is optional (no obvious from the
> >    iio_push_to_buffers_with_timestamp call) we can end up having
> >    to shift the rest of the data within that call.
> >
> > Changes since v1.
> >
> > Andy Schevchenko pointed out that on x86_32 s64 elements are only
> > aligned to 4 bytes.  Where I had tried to use a structure to avoid
> > explicit need to list the padding, there were some cases where
> > this results in insufficient padding being inserted.
> >
> > This doesn't affect the few patches that had already been applied and
> > sent upstream. (which was lucky ;)
> >
> > The fix was to take advantage of __aligned(8) which (according to
> > my reading of the c spec and the gcc docs) enforces the alignment of
> > both the element within a structure and the structure itself.
> > The kernel now requires a recent enough version of GCC to ensure this
> > works both on the stack and heap.  This is done in lots of other
> > userspace interfaces. In some cases iio_push_to_buffers_with_ts
> > is aligning data for passing to userspace, be it via a kfifo
> > so it is sensible we should use the same solution.
> >
> > Note that we could have used u64_aligned but there is no equivalent
> > for s64 and explicit use of __aligned(8) is common in
> > the kernel so we adopt this here.
> >
> > Note that there were about 8 drivers that would have been broken with
> > v1 of the patch.  I have also forced alignment of timestamps in cases
> > where (mostly by coincidence) we would have been fine (padding was
> > less than 4 bytes anyway.  I did this partly to reduce fragility if
> > other elements are added in future and also to avoid cut and paste
> > errors in new drivers.
> >
> > There were a few other minor tidying up changes inline with reviews
> > of v1.
> >
> > I've kept tags given for v1 on basis the changes are minor. Shout if
> > you disagree.
> >
> > Version 1 part 1 cover letter.
> >
> > Lars noted in a recent review [1] of the adis16475 that we had an issue around
> > the alignment requirements of iio_push_to_buffers_with_timestamp.
> > Whilst it's not documented, that function assumes that the overall buffer
> > is 8 byte aligned, to ensure the timestamp is itself naturally aligned.
> > We have drivers that use arrays (typically on the stack) that do
> > not guarantee this alignment.
> >
> > We could have fixed this by using a put_unaligned to write the timestamp
> > but I think that just pushes the problem down the line.  If we were to
> > have a consumer buffer wanting all the channels in the current
> > active_scanmask then it will get the raw buffer from the driver passed
> > straight through.  It seems odd to me if we allow passing a buffer
> > that is not naturally aligned through to a consumer.
> > Hence I'm proposing to fix up all existing drivers that might pass
> > a buffer with insufficient alignment guarantees.
> > Sometimes the timestamp is guaranteed to be in a particular location,
> > in which case we can use C structure alignment guarantees to fix this
> > in a nice readable fashion.  In other cases, the timestamp location
> > depends on which channels are enabled, and in those case we can
> > use explicit alignment __aligned(8) to ensure the whole array is
> > appropriately aligned.
> >
> > Lars-Peter also noted that, in many of these cases, there are holes
> > in the stack array that we never write.  Those provide a potential
> > leak of kernel data to userspace.  For drivers where this applies
> > we either need to zero those holes each time, or allocate the buffer
> > on the heap (only once), ensuring it is zeroed at that time.
> > We may leak previous values from the sensor but currently that seems
> > unlikely to present any form of security risk.
> >
> > As such, this first set contains a mixture of fixes.  Where there
> > are no possible holes, the buffer is kept on the stack but a
> > c structure is used to guarantee appropriate alignment.  Where
> > there are holes, the buffer is moved into the iio_priv() accessed
> > data private structure. A c structure or __aligned(8) is used
> > as appropriate.
> >
> > I've stopped at this point rather than doing all the drivers Lars
> > found in order to both throttle the review burden and also to
> > see find any general problems with the fixes before doign futher
> > similar series.  A few of the remaining ones will be rather more
> > complex to deal with.
> >
> > These have been there a long time, so whilst they are fixes we
> > will want in stable I'm not that bothered if it takes us a little
> > while to get them there!
> >
> > [1] https://www.spinics.net/lists/devicetree/msg350590.html
> > [2] https://patchwork.kernel.org/cover/11554215/
> >
> > Jonathan Cameron (8):
> >   iio:light:rpr0521: Fix timestamp alignment and prevent data leak.
> >   iio:light:st_uvis25: Fix timestamp alignment and prevent data leak.
> >   iio:magnetometer:mag3110: Fix alignment and data leak issues.
> >   iio:imu:bmi160: Fix too large a buffer.
> >   iio:imu:bmi160: Fix alignment and data leak issues
> >   iio:pressure:mpl3115: Force alignment of buffer
> >   iio:adc:ti-ads124s08: Fix buffer being too long.
> >   iio:adc:ti-ads124s08: Fix alignment and data leak issues.
> >
> >  drivers/iio/adc/ti-ads124s08.c       | 13 ++++++++++---
> >  drivers/iio/imu/bmi160/bmi160.h      |  7 +++++++
> >  drivers/iio/imu/bmi160/bmi160_core.c |  6 ++----
> >  drivers/iio/light/rpr0521.c          | 17 +++++++++++++----
> >  drivers/iio/light/st_uvis25.h        |  5 +++++
> >  drivers/iio/light/st_uvis25_core.c   |  8 +++++---
> >  drivers/iio/magnetometer/mag3110.c   | 13 +++++++++----
> >  drivers/iio/pressure/mpl3115.c       |  9 ++++++++-
> >  8 files changed, 59 insertions(+), 19 deletions(-)
> >
>