Re: [PATCH 0/7] iio: fix possible race condition during access of available info lists

[Jonathan Cameron]

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,