| Message ID | 20241015-iio-read-avail-release-v3-2-ac3e08f25cb3@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: fix possible race condition during access of available info lists | expand |
Hi, On Tue, Oct 15, 2024 at 01:06:35PM +0200, Matteo Martelli wrote: > Consumers need to call the producer's read_avail_release_resource() > callback after reading producer's available info. To avoid a race > condition with the producer unregistration, change inkern > iio_channel_read_avail() so that it copies the available info from the > producer and immediately calls its release callback with info_exists > locked. > > Also, modify the users of iio_read_avail_channel_raw() and > iio_read_avail_channel_attribute() to free the copied available buffers > after calling these functions. > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > --- > diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c > index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644 > --- a/drivers/power/supply/ingenic-battery.c > +++ b/drivers/power/supply/ingenic-battery.c > @@ -12,6 +12,7 @@ > #include <linux/platform_device.h> > #include <linux/power_supply.h> > #include <linux/property.h> > +#include <linux/slab.h> > > struct ingenic_battery { > struct device *dev; > @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > dev_err(bat->dev, "Unable to read channel avail scale\n"); > return ret; > } > - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) > - return -EINVAL; > + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) { > + ret = -EINVAL; > + goto out; > + } > > max_mV = bat->info->voltage_max_design_uv / 1000; > > @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > if (best_idx < 0) { > dev_err(bat->dev, "Unable to find matching voltage scale\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto out; > } > > /* Only set scale if there is more than one (fractional) entry */ > @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > scale_raw[best_idx + 1], > IIO_CHAN_INFO_SCALE); > if (ret) > - return ret; > + goto out; > } > > - return 0; > + ret = 0; > +out: > + kfree(scale_raw); > + return ret; > } > > static enum power_supply_property ingenic_battery_properties[] = { It should be enough to declare scale_raw like this at the beginning of the function and otherwise keep it as is when you include <linux/cleanup.h>: const int *scale_raw __free(kfree) = NULL; Greetings, -- Sebastian
Quoting Sebastian Reichel (2024-10-16 23:08:30) > Hi, > > On Tue, Oct 15, 2024 at 01:06:35PM +0200, Matteo Martelli wrote: > > Consumers need to call the producer's read_avail_release_resource() > > callback after reading producer's available info. To avoid a race > > condition with the producer unregistration, change inkern > > iio_channel_read_avail() so that it copies the available info from the > > producer and immediately calls its release callback with info_exists > > locked. > > > > Also, modify the users of iio_read_avail_channel_raw() and > > iio_read_avail_channel_attribute() to free the copied available buffers > > after calling these functions. > > > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > > --- > > diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c > > index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644 > > --- a/drivers/power/supply/ingenic-battery.c > > +++ b/drivers/power/supply/ingenic-battery.c > > @@ -12,6 +12,7 @@ > > #include <linux/platform_device.h> > > #include <linux/power_supply.h> > > #include <linux/property.h> > > +#include <linux/slab.h> > > > > struct ingenic_battery { > > struct device *dev; > > @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > dev_err(bat->dev, "Unable to read channel avail scale\n"); > > return ret; > > } > > - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) > > - return -EINVAL; > > + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) { > > + ret = -EINVAL; > > + goto out; > > + } > > > > max_mV = bat->info->voltage_max_design_uv / 1000; > > > > @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > > > if (best_idx < 0) { > > dev_err(bat->dev, "Unable to find matching voltage scale\n"); > > - return -EINVAL; > > + ret = -EINVAL; > > + goto out; > > } > > > > /* Only set scale if there is more than one (fractional) entry */ > > @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > scale_raw[best_idx + 1], > > IIO_CHAN_INFO_SCALE); > > if (ret) > > - return ret; > > + goto out; > > } > > > > - return 0; > > + ret = 0; > > +out: > > + kfree(scale_raw); > > + return ret; > > } > > > > static enum power_supply_property ingenic_battery_properties[] = { > > It should be enough to declare scale_raw like this at the beginning > of the function and otherwise keep it as is when you include > <linux/cleanup.h>: > > const int *scale_raw __free(kfree) = NULL; Nice! I wasn't aware of it, thanks! I'll try it and submit it in next version. I think that also fits for the similar usage in iio_channel_read_min() and iio_channel_read_max() as well. > > Greetings, > > -- Sebastian Thanks, Matteo Martelli
On Thu, 17 Oct 2024 12:49:23 +0200 Matteo Martelli <matteomartelli3@gmail.com> wrote: > Quoting Sebastian Reichel (2024-10-16 23:08:30) > > Hi, > > > > On Tue, Oct 15, 2024 at 01:06:35PM +0200, Matteo Martelli wrote: > > > Consumers need to call the producer's read_avail_release_resource() > > > callback after reading producer's available info. To avoid a race > > > condition with the producer unregistration, change inkern > > > iio_channel_read_avail() so that it copies the available info from the > > > producer and immediately calls its release callback with info_exists > > > locked. > > > > > > Also, modify the users of iio_read_avail_channel_raw() and > > > iio_read_avail_channel_attribute() to free the copied available buffers > > > after calling these functions. > > > > > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> > > > --- > > > diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c > > > index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644 > > > --- a/drivers/power/supply/ingenic-battery.c > > > +++ b/drivers/power/supply/ingenic-battery.c > > > @@ -12,6 +12,7 @@ > > > #include <linux/platform_device.h> > > > #include <linux/power_supply.h> > > > #include <linux/property.h> > > > +#include <linux/slab.h> > > > > > > struct ingenic_battery { > > > struct device *dev; > > > @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > > dev_err(bat->dev, "Unable to read channel avail scale\n"); > > > return ret; > > > } > > > - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) > > > - return -EINVAL; > > > + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) { > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > > > > max_mV = bat->info->voltage_max_design_uv / 1000; > > > > > > @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > > > > > if (best_idx < 0) { > > > dev_err(bat->dev, "Unable to find matching voltage scale\n"); > > > - return -EINVAL; > > > + ret = -EINVAL; > > > + goto out; > > > } > > > > > > /* Only set scale if there is more than one (fractional) entry */ > > > @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) > > > scale_raw[best_idx + 1], > > > IIO_CHAN_INFO_SCALE); > > > if (ret) > > > - return ret; > > > + goto out; > > > } > > > > > > - return 0; > > > + ret = 0; > > > +out: > > > + kfree(scale_raw); > > > + return ret; > > > } > > > > > > static enum power_supply_property ingenic_battery_properties[] = { > > > > It should be enough to declare scale_raw like this at the beginning > > of the function and otherwise keep it as is when you include > > <linux/cleanup.h>: > > > > const int *scale_raw __free(kfree) = NULL; > > Nice! I wasn't aware of it, thanks! I'll try it and submit it in next version. > > I think that also fits for the similar usage in iio_channel_read_min() and > iio_channel_read_max() as well. Take care with this + read the documents. The constructor and destructor should be in one line. https://lore.kernel.org/all/172294149613.2215.3274492813920223809.tip-bot2@tip-bot2/ specifically the second to last line. It's a clever tool but use with care! Jonathan > > > > > Greetings, > > > > -- Sebastian > > Thanks, > Matteo Martelli
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 56e5913ab82d1c045c9ca27012008a4495502cbf..78bb86c291706748b4072a484532ad20c415ff9f 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -249,9 +249,17 @@ static int rescale_read_avail(struct iio_dev *indio_dev, } } +static void rescale_read_avail_release_res(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int *vals, long mask) +{ + kfree(vals); +} + static const struct iio_info rescale_info = { .read_raw = rescale_read_raw, .read_avail = rescale_read_avail, + .read_avail_release_resource = rescale_read_avail_release_res, }; static ssize_t rescale_read_ext_info(struct iio_dev *indio_dev, diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c index f36f10bfb6be7863a56b911b5f58671ef530c977..43d68e17fc3a5fca59fad6ccf818eeadfecdb8c1 100644 --- a/drivers/iio/dac/dpot-dac.c +++ b/drivers/iio/dac/dpot-dac.c @@ -108,6 +108,13 @@ static int dpot_dac_read_avail(struct iio_dev *indio_dev, return -EINVAL; } +static void dpot_dac_read_avail_release_res(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int *vals, long mask) +{ + kfree(vals); +} + static int dpot_dac_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val, int val2, long mask) @@ -125,6 +132,7 @@ static int dpot_dac_write_raw(struct iio_dev *indio_dev, static const struct iio_info dpot_dac_info = { .read_raw = dpot_dac_read_raw, .read_avail = dpot_dac_read_avail, + .read_avail_release_resource = dpot_dac_read_avail_release_res, .write_raw = dpot_dac_write_raw, }; diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c index 7f325b3ed08fae6674245312cf8f57bb151006c0..7f50e33dc5084673aa66c25731add0c314cb477d 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -760,9 +760,29 @@ static int iio_channel_read_avail(struct iio_channel *chan, if (!iio_channel_has_available(chan->channel, info)) return -EINVAL; - if (iio_info->read_avail) - return iio_info->read_avail(chan->indio_dev, chan->channel, - vals, type, length, info); + if (iio_info->read_avail) { + const int *vals_tmp; + int ret; + + ret = iio_info->read_avail(chan->indio_dev, chan->channel, + &vals_tmp, type, length, info); + if (ret < 0) + return ret; + + /* + * Copy the producer's avail buffer with lock_exists locked to + * avoid possible race with producer unregistration. + */ + *vals = kmemdup_array(vals_tmp, *length, sizeof(int), GFP_KERNEL); + if (!*vals) + return -ENOMEM; + + if (iio_info->read_avail_release_resource) + iio_info->read_avail_release_resource( + chan->indio_dev, chan->channel, vals_tmp, info); + + return ret; + } return -EINVAL; } @@ -789,9 +809,11 @@ int iio_read_avail_channel_raw(struct iio_channel *chan, ret = iio_read_avail_channel_attribute(chan, vals, &type, length, IIO_CHAN_INFO_RAW); - if (ret >= 0 && type != IIO_VAL_INT) + if (ret >= 0 && type != IIO_VAL_INT) { /* raw values are assumed to be IIO_VAL_INT */ + kfree(*vals); ret = -EINVAL; + } return ret; } @@ -820,24 +842,31 @@ static int iio_channel_read_max(struct iio_channel *chan, if (val2) *val2 = vals[5]; } - return 0; + ret = 0; + break; case IIO_AVAIL_LIST: - if (length <= 0) - return -EINVAL; + if (length <= 0) { + ret = -EINVAL; + goto out; + } switch (*type) { case IIO_VAL_INT: *val = max_array(vals, length); + ret = 0; break; default: /* TODO: learn about max for other iio values */ - return -EINVAL; + ret = -EINVAL; } - return 0; + break; default: - return -EINVAL; + ret = -EINVAL; } +out: + kfree(vals); + return ret; } int iio_read_max_channel_raw(struct iio_channel *chan, int *val) @@ -876,24 +905,31 @@ static int iio_channel_read_min(struct iio_channel *chan, if (val2) *val2 = vals[1]; } - return 0; + ret = 0; + break; case IIO_AVAIL_LIST: - if (length <= 0) - return -EINVAL; + if (length <= 0) { + ret = -EINVAL; + goto out; + } switch (*type) { case IIO_VAL_INT: *val = min_array(vals, length); + ret = 0; break; default: /* TODO: learn about min for other iio values */ - return -EINVAL; + ret = -EINVAL; } - return 0; + break; default: - return -EINVAL; + ret = -EINVAL; } +out: + kfree(vals); + return ret; } int iio_read_min_channel_raw(struct iio_channel *chan, int *val) diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..31345437784b01c5d6f8ea70263f4c2574388e7a 100644 --- a/drivers/iio/multiplexer/iio-mux.c +++ b/drivers/iio/multiplexer/iio-mux.c @@ -142,6 +142,13 @@ static int mux_read_avail(struct iio_dev *indio_dev, return ret; } +static void mux_read_avail_release_res(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int *vals, long mask) +{ + kfree(vals); +} + static int mux_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int val, int val2, long mask) @@ -171,6 +178,7 @@ static int mux_write_raw(struct iio_dev *indio_dev, static const struct iio_info mux_info = { .read_raw = mux_read_raw, .read_avail = mux_read_avail, + .read_avail_release_resource = mux_read_avail_release_res, .write_raw = mux_write_raw, }; diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..3db000d9fff9a7a6819631314547b3d16db7f967 100644 --- a/drivers/power/supply/ingenic-battery.c +++ b/drivers/power/supply/ingenic-battery.c @@ -12,6 +12,7 @@ #include <linux/platform_device.h> #include <linux/power_supply.h> #include <linux/property.h> +#include <linux/slab.h> struct ingenic_battery { struct device *dev; @@ -79,8 +80,10 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) dev_err(bat->dev, "Unable to read channel avail scale\n"); return ret; } - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) - return -EINVAL; + if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) { + ret = -EINVAL; + goto out; + } max_mV = bat->info->voltage_max_design_uv / 1000; @@ -99,7 +102,8 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) if (best_idx < 0) { dev_err(bat->dev, "Unable to find matching voltage scale\n"); - return -EINVAL; + ret = -EINVAL; + goto out; } /* Only set scale if there is more than one (fractional) entry */ @@ -109,10 +113,13 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) scale_raw[best_idx + 1], IIO_CHAN_INFO_SCALE); if (ret) - return ret; + goto out; } - return 0; + ret = 0; +out: + kfree(scale_raw); + return ret; } static enum power_supply_property ingenic_battery_properties[] = { diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h index 333d1d8ccb37f387fe531577ac5e0bfc7f752cec..e3e268d2574b3e01c9412449d90d627de7efcd84 100644 --- a/include/linux/iio/consumer.h +++ b/include/linux/iio/consumer.h @@ -316,7 +316,7 @@ int iio_read_min_channel_raw(struct iio_channel *chan, int *val); /** * iio_read_avail_channel_raw() - read available raw values from a given channel * @chan: The channel being queried. - * @vals: Available values read back. + * @vals: Available values read back. Must be freed after use. * @length: Number of entries in vals. * * Returns an error code, IIO_AVAIL_RANGE or IIO_AVAIL_LIST. @@ -334,7 +334,7 @@ int iio_read_avail_channel_raw(struct iio_channel *chan, /** * iio_read_avail_channel_attribute() - read available channel attribute values * @chan: The channel being queried. - * @vals: Available values read back. + * @vals: Available values read back. Must be freed after use. * @type: Type of values read back. * @length: Number of entries in vals. * @attribute: info attribute to be read back.
Consumers need to call the producer's read_avail_release_resource() callback after reading producer's available info. To avoid a race condition with the producer unregistration, change inkern iio_channel_read_avail() so that it copies the available info from the producer and immediately calls its release callback with info_exists locked. Also, modify the users of iio_read_avail_channel_raw() and iio_read_avail_channel_attribute() to free the copied available buffers after calling these functions. Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com> --- drivers/iio/afe/iio-rescale.c | 8 ++++ drivers/iio/dac/dpot-dac.c | 8 ++++ drivers/iio/inkern.c | 68 ++++++++++++++++++++++++++-------- drivers/iio/multiplexer/iio-mux.c | 8 ++++ drivers/power/supply/ingenic-battery.c | 17 ++++++--- include/linux/iio/consumer.h | 4 +- 6 files changed, 90 insertions(+), 23 deletions(-)