From: Matteo Martelli <matteomartelli3@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
Michael Hennerich <Michael.Hennerich@analog.com>,
Alisa-Dariana Roman <alisa.roman@analog.com>,
Christian Eggers <ceggers@arri.de>,
Peter Rosin <peda@axentia.se>,
Paul Cercueil <paul@crapouillou.net>,
Sebastian Reichel <sre@kernel.org>
Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org, linux-pm@vger.kernel.org,
Matteo Martelli <matteomartelli3@gmail.com>
Subject: [PATCH 0/7] iio: fix possible race condition during access of available info lists
Date: Thu, 03 Oct 2024 19:34:05 +0200 [thread overview]
Message-ID: <20241003-iio-read-avail-release-v1-0-c70cc7d9c2e0@gmail.com> (raw)
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,
--
Matteo Martelli <matteomartelli3@gmail.com>
next reply other threads:[~2024-10-03 17:34 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-03 17:34 Matteo Martelli [this message]
2024-10-03 17:34 ` [PATCH 1/7] iio: core: add read_avail_release_resource callback to fix race Matteo Martelli
2024-10-03 17:34 ` [PATCH 2/7] iio: pac1921: use read_avail+release APIs instead of custom ext_info Matteo Martelli
2024-10-03 17:34 ` [PATCH 3/7] iio: ad7192: copy/release available filter frequencies to fix race Matteo Martelli
2024-10-03 17:34 ` [PATCH 4/7] iio: as73211: copy/release available integration times " Matteo Martelli
2024-10-06 14:18 ` Jonathan Cameron
2024-10-03 17:34 ` [PATCH 5/7] iio: inkern: copy/release available info from producer Matteo Martelli
2024-10-03 17:34 ` [PATCH 6/7] iio: consumers: release available info buffer copied " Matteo Martelli
2024-10-06 14:20 ` Jonathan Cameron
2024-10-03 17:34 ` [PATCH 7/7] power: supply: ingenic-battery: free scale buffer after use Matteo Martelli
2024-10-04 21:55 ` kernel test robot
2024-10-04 23:38 ` kernel test robot
2024-10-06 14:23 ` [PATCH 0/7] iio: fix possible race condition during access of available info lists Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241003-iio-read-avail-release-v1-0-c70cc7d9c2e0@gmail.com \
--to=matteomartelli3@gmail.com \
--cc=Michael.Hennerich@analog.com \
--cc=alisa.roman@analog.com \
--cc=ceggers@arri.de \
--cc=jic23@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=paul@crapouillou.net \
--cc=peda@axentia.se \
--cc=sre@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).