| Message ID | 20241003-iio-read-avail-release-v1-0-c70cc7d9c2e0@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: fix possible race condition during access of available info lists | expand |
On Thu, 03 Oct 2024 19:34:05 +0200 Matteo Martelli <matteomartelli3@gmail.com> 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@gmail.com/ > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > --- > 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,
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. 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@gmail.com/ Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> --- 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,