On Thu, 03 Oct 2024 19:34:05 +0200
Matteo Martelli <matteomartelli3@xxxxxxxxx> wrote:
> Some iio drivers currently share an available info list buffer that
> might be changed while iio core prints it to sysfs. This could cause the
> buffer shared with iio core to be corrupted. However, note that I was
> able to trigger the race condition only by adding a delay between each
> sysfs_emit_at calls in the iio_format_list() to force the concurrent
> access to the shared available list buffer.
>
> This patch set extends the iio APIs and fixes some affected drivers.
Thanks for tidying these up. My comments are very minor but
as this is changing how a core bit of infrastructure works I'd like
them to sit on the list another week anyway.
There is just enough here that I'd prefer a v2 though if you don't
get time I can probably tidy it up whilst applying.
The build bot issue is presumably a missing include.
Thanks,
Jonathan
>
> Summary:
> - Patch 1: iio core: introduce a iio info release callback to let
> drivers share a copy of their available info list and later free it.
>
> - Patch 2: pac1921: handle the current scale available info via the
> read_avail+read_avail_release_resource APIs instead of using an ad-hoc
> ext_info attribute. The latter was used to avoid the risk of a race in
> the available list.
>
> - Patch 3,4: ad7192, as73211: fix the possible race in the drivers by
> copying/releasing the affected available lists.
>
> - Patch 5: inkern: make consumers copy and release the available info
> lists of their producers, necessary after patch 1.
>
> - Patch 6,7: iio-mux, iio-rescale, dpot-dac, ingenic-battery: adapt
> consumers to inkern API change by freeing the now copied available
> lists of their producers.
>
> Tested:
> - pac1921: could not reproduce the race condition with the new APIs,
> even with additional delays among the sysfs_emit_at calls during a
> shunt resistor write. No new issue found after the change.
>
> - iio-mux, iio-rescale, dpot-dac: tested with pac1921 as producer, which
> was adapted to produce a mock raw available info list.
> The tests did not cover the driver features but focused on assessing
> the function call sequence. For example the following traced function
> graph shows a read of the dpot mocked out voltage (with ftrace
> filters: pac1921* iio* dpot* kmemdup_array* kfree*):
>
> 3) | iio_read_channel_info_avail [industrialio]() {
> 3) | dpot_dac_read_avail [dpot_dac]() {
> 3) | iio_read_avail_channel_raw [industrialio]() {
> 3) | iio_channel_read_avail [industrialio]() {
> 3) | pac1921_read_avail [pac1921]() {
> 3) 5.208 us | kmemdup_array();
> 3) + 11.459 us | }
> 3) 3.167 us | kmemdup_array();
> 3) | pac1921_read_avail_release_res [pac1921]() {
> 3) 1.709 us | kfree();
> 3) 4.458 us | }
> 3) + 25.750 us | }
> 3) + 31.792 us | }
> 3) + 35.000 us | }
> 3) + 37.083 us | iio_format_list [industrialio]();
> 3) | dpot_dac_read_avail_release_res [dpot_dac]() {
> 3) 1.583 us | kfree();
> 3) 4.250 us | }
> 3) + 84.292 us | }
>
> - ingenic-battery: also tested with mock available info produced by the
> pac1921 driver. Following the traced graph part that should correspond
> to the ingenic_battery_set_scale() flow (which is not traceable with
> the additional ingenic* ftrace filter for some reason):
>
> 2) | ingenic_battery_probe [ingenic_battery]() {
> ...
> 2) | iio_read_max_channel_raw [industrialio]() {
> 2) | iio_channel_read_avail [industrialio]() {
> 2) | pac1921_read_avail [pac1921]() {
> 2) 4.333 us | kmemdup_array();
> 2) + 10.834 us | }
> 2) 3.500 us | kmemdup_array();
> 2) | pac1921_read_avail_release_res [pac1921]() {
> 2) 1.791 us | kfree();
> 2) 4.625 us | }
> 2) + 26.291 us | }
> 2) 1.583 us | kfree();
> 2) + 35.750 us | }
> 2) | iio_read_avail_channel_attribute [industrialio]() {
> 2) | iio_channel_read_avail [industrialio]() {
> 2) | pac1921_read_avail [pac1921]() {
> 2) 3.250 us | kmemdup_array();
> 2) 8.209 us | }
> 2) 3.458 us | kmemdup_array();
> 2) | pac1921_read_avail_release_res [pac1921]() {
> 2) 1.542 us | kfree();
> 2) 4.292 us | }
> 2) + 21.417 us | }
> 2) + 26.333 us | }
> 2) | iio_write_channel_attribute [industrialio]() {
> 2) 4.375 us | pac1921_write_raw [pac1921]();
> 2) 9.625 us | }
> 2) 1.666 us | kfree();
> 2) * 47810.08 us | }
>
> Not tested:
> - ad7192, as73211
>
> Link: https://lore.kernel.org/linux-iio/20240724-iio-pac1921-v4-0-723698e903a3@xxxxxxxxx/
>
> Signed-off-by: Matteo Martelli <matteomartelli3@xxxxxxxxx>
> ---
> Matteo Martelli (7):
> iio: core: add read_avail_release_resource callback to fix race
> iio: pac1921: use read_avail+release APIs instead of custom ext_info
> iio: ad7192: copy/release available filter frequencies to fix race
> iio: as73211: copy/release available integration times to fix race
> iio: inkern: copy/release available info from producer
> iio: consumers: release available info buffer copied from producer
> power: supply: ingenic-battery: free scale buffer after use
>
> drivers/iio/adc/ad7192.c | 22 +++++-
> drivers/iio/adc/pac1921.c | 128 ++++++++++++---------------------
> drivers/iio/afe/iio-rescale.c | 8 +++
> drivers/iio/dac/dpot-dac.c | 8 +++
> drivers/iio/industrialio-core.c | 14 +++-
> drivers/iio/inkern.c | 64 ++++++++++++-----
> drivers/iio/light/as73211.c | 23 +++++-
> drivers/iio/multiplexer/iio-mux.c | 8 +++
> drivers/power/supply/ingenic-battery.c | 16 +++--
> include/linux/iio/consumer.h | 4 +-
> include/linux/iio/iio.h | 4 ++
> 11 files changed, 185 insertions(+), 114 deletions(-)
> ---
> base-commit: fec496684388685647652ab4213454fbabdab099
> change-id: 20240802-iio-read-avail-release-cb3d2a1e1b98
>
> Best regards,
