| Message ID | 20241021-iio-read-avail-release-v5-2-b168713fab33@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | iio: fix possible race condition during access of available info lists | expand |
On Wed, 30 Oct 2024 16:47:50 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Mon, Oct 21, 2024 at 02:54:15PM +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. To let users free the copied buffer with > > a cleanup pattern, also add a iio_read_avail_channel_attr_retvals() > > consumer helper that is equivalent to iio_read_avail_channel_attribute() > > but stores the available values in the returned variable. > > ... > > > +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, > > }; > > I have a problem with this approach. The issue is that we allocate > memory in one place and must clear it in another. This is not well > designed thingy in my opinion. It is a tricky corner and we've not yet come up with a better solution :( I think one of the earlier versions did just always copy and the reviews suggested that was painful given we are fixing a tiny percentage of devices. Hence we ended up with what is effectively an optional copy if the provider knows the data is volatile. So there are two 'potential' copies here and we need to be careful to separate them for purposes of discussion. A) Copy in provider if it has volatile available data. In that case the copy is done in a call to it via read_avail, and release via a call to read_avail_release_resource(). So to my mind locally the same as any acquire / release pair. B) Copy in the core for the case where we need the lifetime to persist. That is a effectively a kmemdup() call so we could call back to the core to release it but it would just be a kfree() wrapper. (A) Only occurs in a tiny subset of drivers, most use non volatile data for read avail (constant, or constant after probe). (B) Only occurs for consumer drivers that directly use the avail data. There are very few of those and no other sane way of solving this because we can't hold a lock into the provider for an unknown (long) time. > I was thinking a bit of the solution and > at least these two comes to my mind: > > 1) having a special callback for .read_avail_with_copy (choose better > name) that will dump the data to the intermediate buffer and clean it > after all; So we have that allocate the data in the provider and hand it to the consumer which then frees it with kfree() in all cases? Note that's what we do for the inkern interfaces (the ones consumer drivers have to use), just in the core not the providers because that corner is hard to close any other way. In this rare case we end up potentially copying twice. For the special cases where the buffer isn't passed on beyond functions that are part of the IIO core, we avoid the need for that (potentially second, probably only) copy because we can always ensure the release call is made. Note this is the common case by far. It's the pretty printing done by the core to present this data to sysfs etc, or to find out the max value for some consumer that doesn't need the whole set. > > 2) introduce a new type (or bit there), like IIO_AVAIL_LIST_ALLOC. The special handling that will need seems likely to be no more obvious than the handling we have here. I'm not really sure how it would work. > > In any case it looks fragile and not scalable. I propose to drop this > and think again. > > Yes, yes, I'm fully aware about the problem you are trying to solve and > agree on the report, I think this solution is not good enough. I'll back this out of my tree for now so the discussion can carry on. Jonathan
Quoting Jonathan Cameron (2024-10-31 15:31:29) > On Thu, 31 Oct 2024 12:26:24 +0100 > Matteo Martelli <matteomartelli3@gmail.com> wrote: > > > Quoting Jonathan Cameron (2024-10-30 21:30:50) > > > On Wed, 30 Oct 2024 19:23:21 +0100 > > > Matteo Martelli <matteomartelli3@gmail.com> wrote: > > > > > > > Quoting Andy Shevchenko (2024-10-30 15:47:50) > > > > > On Mon, Oct 21, 2024 at 02:54:15PM +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. To let users free the copied buffer with > > > > > > a cleanup pattern, also add a iio_read_avail_channel_attr_retvals() > > > > > > consumer helper that is equivalent to iio_read_avail_channel_attribute() > > > > > > but stores the available values in the returned variable. > > > > > > > > > > ... > > > > > > > > > > > +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, > > > > > > }; > > > > > > > > > > I have a problem with this approach. The issue is that we allocate > > > > > memory in one place and must clear it in another. This is not well > > > > > designed thingy in my opinion. I was thinking a bit of the solution and > > > > > at least these two comes to my mind: > > > > > > > > > > 1) having a special callback for .read_avail_with_copy (choose better > > > > > name) that will dump the data to the intermediate buffer and clean it > > > > > after all; > > > > > > > > > > 2) introduce a new type (or bit there), like IIO_AVAIL_LIST_ALLOC. > > > > > > > > Could you elaborate more about these potential solutions? Maybe with some > > > > usage examples? > > > > > > > > If I get it correctly, in both cases you are suggesting to pass ownership > > > > of the vals buffer to the caller, iio_read_channel_info_avail() in this > > > > case, so that it would take care of freeing the buffer after calling > > > > iio_format_after_*(). We considered this approach during an initial > > > > discussion with Jonathan (see read_avail_ext() in [1]), where he suggested > > > > to let the driver keep the release control through a callback for two > > > > reasons: > > > > > > > > 1) Apparently it's a bad pattern to pass the buffer ownership to the core, > > > > maybe Jonathan can elaborate why? The risk I can think of is that the driver > > > > could still keep the buffer copy in its private data after giving it away, > > > > resulting in fact in a double ownership. However I think it would be clear > > > > enough in this case that the copy should be handled by the caller, or maybe > > > > not? > > > Mostly the lack of desire to have to copy for the 95% of cases where it's > > > not needed and that it prevents any optimization like you mention. > > > > I think the suggestion here is to add an additional .read_avail_with_copy() > > without replacing the original .read_avail(), so all the current drivers that > > use a constant avail list would not be affected. And I think this was the same > > idea for the additional read_avail_ext() or the additional argument for the > > read_avail() we were considering in [1]. So I would think that > > iio_read_channel_info_avail() would do something like the following: > > > > if (indio_dev->info->read_avail_with_copy) > > indio_dev->info->read_avail_with_copy(vals); > > else > > indio_dev->info->read_avail(vals); > > > > ... > > iio_format_avail_list(vals); > > ... > > > > if (indio_dev->info->read_avail_with_copy) > > kfree(vals); > > Ok, sure that would work, but... > > I don't really see this as being much less fragile than > the existing solution + in cases that we do have where > only some available are not const we will have to copy them > all. > > If anything it's more complex than making it a driver problem > to provide the release call however it wants to do it. > > > > > > And the drivers would choose whether to define the read_avail or the > > read_avail_with_copy. > > > > What I was referring to is that, back then, you mentioned you would have > > preferred to avoid passing ownership of the buffer around: > > > > > That's a corner case we should think about closing. Would require an indicator > > > to read_avail that the buffer it has been passed is a snapshot that it should > > > free on completion of the string building. I don't like passing ownership > > > of data around like that, but it is fiddly to do anything else given > > > any simple double buffering is subject to race conditions. > > > > I guess there is some other reason other than avoiding the copy when not > > necessary, since by introducing an additional function or argument or return > > type, most of the unnecessary copies would already be avoided right? > > It's not a strong reason beyond limiting scope of clever design + > the key bit my mind is that the above is not substantially simpler and > reduces our flexibility. > > > > > Anyway any of this solutions would still prevent the potential optimizations of > > point 2). It's worth mentioning that those kind of optimizations are currently > > not adopted by any driver. > > That one indeed not, but mixing dynamic and non dynamic is something > you do in your pac1921 patch. Good point! I didn't think about it, or more likely I forgot, that with an additional read_avail_with_copy() used as in the example you cannot mix dynamic and non dynamic available lists, thus those drivers that need at least one dynamic available list would always copy all of them as they need to rely to the read_avail_with_copy(). I guess this could be worked around with an additional return argument for the read_avail() or an additional type like the IIO_AVAIL_LIST_ALLOC suggested by Andy to signal the caller it needs to free the list after use. However, I think they would introduce a more invasive change in the current API compared to an additional optional callback, so I agree that the current release callback is still a better option. > > Jonathan > > > > > > > > > > Jonathan > > > > > > > > 2) Some driver might want to avoid allocating a new copy of a big table if > > > > the race does not occur (e.g. with additional checks on buffer access > > > > code) and thus wouldn't call a free() in the release callback. > > > > > > > > > > > > > > In any case it looks fragile and not scalable. I propose to drop this > > > > > and think again. > > > > > > > > I see your concerns, I am open to reconsider this in case we come up with > > > > better solution after addressing the points above. > > > > > > > > > Yes, yes, I'm fully aware about the problem you are trying to solve and > > > > > agree on the report, I think this solution is not good enough. > > > > > > > > > > -- > > > > > With Best Regards, > > > > > Andy Shevchenko > > > > > > > > > > > > > [1]: https://lore.kernel.org/linux-iio/20240729211100.0d602d6e@jic23-huawei/ > > > > > > > > Best regards, > > > > Matteo Martelli > > > > > > > I hope I've brought a little more clarity to the discussion by providing some > > history instead of making it more confusing. > > Sure, the code example in particular is useful. > > Jonathan > > > > > Best regards, > > Matteo Martelli > > > > > Best regards, 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..7b87d1c57d6fd1258d254192835aa6cb9355f859 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -760,12 +760,56 @@ 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; } +/* + * iio_channel_read_avail_retvals() is equivalent to iio_channel_read_avail() + * but stores the pointer to the buffer of available values in the returned + * variable. Since such buffer must be freed after use, this function lets the + * user declare a cleanup local variable, e.g.: + * const int *vals = __free(kfree) = iio_channel_read_avail_retvals(...); + */ +static const int *iio_channel_read_avail_retvals(struct iio_channel *chan, + int *type, int *length, + int *avail_type, + enum iio_chan_info_enum info) +{ + const int *vals; + int ret; + + ret = iio_channel_read_avail(chan, &vals, type, length, info); + if (ret < 0) + return ERR_PTR(ret); + + *avail_type = ret; + + return vals; +} + int iio_read_avail_channel_attribute(struct iio_channel *chan, const int **vals, int *type, int *length, enum iio_chan_info_enum attribute) @@ -780,6 +824,25 @@ int iio_read_avail_channel_attribute(struct iio_channel *chan, } EXPORT_SYMBOL_GPL(iio_read_avail_channel_attribute); +const int * +iio_read_avail_channel_attr_retvals(struct iio_channel *chan, int *type, + int *length, int *avail_type, + enum iio_chan_info_enum attribute) +{ + const int *vals; + int ret; + + ret = iio_read_avail_channel_attribute(chan, &vals, type, length, + attribute); + if (ret < 0) + return ERR_PTR(ret); + + *avail_type = ret; + + return vals; +} +EXPORT_SYMBOL_GPL(iio_read_avail_channel_attr_retvals); + int iio_read_avail_channel_raw(struct iio_channel *chan, const int **vals, int *length) { @@ -789,9 +852,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; } @@ -801,15 +866,16 @@ static int iio_channel_read_max(struct iio_channel *chan, int *val, int *val2, int *type, enum iio_chan_info_enum info) { - const int *vals; int length; - int ret; + int avail_type; - ret = iio_channel_read_avail(chan, &vals, type, &length, info); - if (ret < 0) - return ret; + const int *vals __free(kfree) = + iio_channel_read_avail_retvals(chan, type, &length, + &avail_type, info); + if (IS_ERR(vals)) + return PTR_ERR(vals); - switch (ret) { + switch (avail_type) { case IIO_AVAIL_RANGE: switch (*type) { case IIO_VAL_INT: @@ -857,15 +923,16 @@ static int iio_channel_read_min(struct iio_channel *chan, int *val, int *val2, int *type, enum iio_chan_info_enum info) { - const int *vals; int length; - int ret; + int avail_type; - ret = iio_channel_read_avail(chan, &vals, type, &length, info); - if (ret < 0) - return ret; + const int *vals __free(kfree) = + iio_channel_read_avail_retvals(chan, type, &length, + &avail_type, info); + if (IS_ERR(vals)) + return PTR_ERR(vals); - switch (ret) { + switch (avail_type) { case IIO_AVAIL_RANGE: switch (*type) { case IIO_VAL_INT: 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..5d82c799ba5af3302bfcdfadb5f57adf6b12b353 100644 --- a/drivers/power/supply/ingenic-battery.c +++ b/drivers/power/supply/ingenic-battery.c @@ -6,12 +6,14 @@ * based on drivers/power/supply/jz4740-battery.c */ +#include <linux/cleanup.h> #include <linux/iio/consumer.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/power_supply.h> #include <linux/property.h> +#include <linux/slab.h> struct ingenic_battery { struct device *dev; @@ -62,8 +64,8 @@ static int ingenic_battery_get_property(struct power_supply *psy, */ static int ingenic_battery_set_scale(struct ingenic_battery *bat) { - const int *scale_raw; - int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret; + int scale_len, scale_type, scale_avail_type; + int best_idx = -1, best_mV, max_raw, i, ret; u64 max_mV; ret = iio_read_max_channel_raw(bat->channel, &max_raw); @@ -72,14 +74,18 @@ static int ingenic_battery_set_scale(struct ingenic_battery *bat) return ret; } - ret = iio_read_avail_channel_attribute(bat->channel, &scale_raw, - &scale_type, &scale_len, - IIO_CHAN_INFO_SCALE); - if (ret < 0) { + const int *scale_raw __free(kfree) = + iio_read_avail_channel_attr_retvals(bat->channel, + &scale_type, + &scale_len, + &scale_avail_type, + IIO_CHAN_INFO_SCALE); + if (IS_ERR(scale_raw)) { dev_err(bat->dev, "Unable to read channel avail scale\n"); - return ret; + return PTR_ERR(scale_raw); } - if (ret != IIO_AVAIL_LIST || scale_type != IIO_VAL_FRACTIONAL_LOG2) + if (scale_avail_type != IIO_AVAIL_LIST || + scale_type != IIO_VAL_FRACTIONAL_LOG2) return -EINVAL; max_mV = bat->info->voltage_max_design_uv / 1000; diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h index 333d1d8ccb37f387fe531577ac5e0bfc7f752cec..188cc64609bd1fd8e0522e69f0b65a73b9b1606d 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. @@ -345,6 +345,30 @@ int iio_read_avail_channel_attribute(struct iio_channel *chan, const int **vals, int *type, int *length, enum iio_chan_info_enum attribute); + +/** + * iio_read_avail_channel_attr_retvals() - read available channel attr values + * @chan: The channel being queried. + * @type: Type of values read back. + * @length: Number of entries in vals. + * @avail_type: Available type of values read back + * (IIO_AVAIL_RANGE or IIO_AVAIL_LIST). + * @attribute: info attribute to be read back. + * + * This function is equivalent to iio_read_avail_channel_attribute() but stores + * the pointer to the buffer of available values in the returned variable. + * Since such buffer must be freed after use, this function lets the user + * declare a cleanup local variable, e.g.: + * const int *vals = __free(kfree) = iio_read_avail_channel_attr_retvals(...); + * + * Returns a pointer to negative errno on error otherwise returns the available + * values read back that must be freed after use. + */ +const int * +iio_read_avail_channel_attr_retvals(struct iio_channel *chan, int *type, + int *length, int *avail_type, + enum iio_chan_info_enum attribute); + /** * iio_get_channel_type() - get the type of a channel * @channel: The channel being queried.
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. To let users free the copied buffer with a cleanup pattern, also add a iio_read_avail_channel_attr_retvals() consumer helper that is equivalent to iio_read_avail_channel_attribute() but stores the available values in the returned variable. 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 | 99 ++++++++++++++++++++++++++++------ drivers/iio/multiplexer/iio-mux.c | 8 +++ drivers/power/supply/ingenic-battery.c | 22 +++++--- include/linux/iio/consumer.h | 28 +++++++++- 6 files changed, 147 insertions(+), 26 deletions(-)