Re: [PATCH v4 2/5] iio: consumers: copy/release available info from producer to fix race

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Oct 2024 12:16:41 +0200
Matteo Martelli <matteomartelli3@gmail.com> 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>
Hi Matteo,

The cleanup.h stuff is new and comes with footguns. As such the
'rules' applied are perhaps stricter than then will be in the long term.
https://lore.kernel.org/all/172294149613.2215.3274492813920223809.tip-bot2@tip-bot2/
is what we have today. Particularly the last few paragraphs on usage.

> @@ -857,7 +879,7 @@ 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;
> +	const int *vals __free(kfree) = NULL;

Unlike below this one is 'almost' ok because there isn't much below. However,
still not good because of the risk of future code putting something in between.
At very minimum move it down to just before the place it's allocated.

It's a bit messy but maybe what we need is:

int *iio_read_avail_channel_attribute_retvals(struct iio_channel *chan,
				     	      int *type, int *length,
				              enum iio_chan_info_enum attr)
{
	int *vals;
	int ret;

	ret = iio_read_avail_channel_attribute(chan, &vals, type, len, attr;
	if (ret)
		return ERR_PTR(ret);

	return vals;
}

Then you can do
	const int *vals __free(kfree) =
		iio_channel_read_avail_retvals(chan, type, &length, info);

	if (IS_ERR(vals))
		...

and obey the suggested style for cleanup.h usage.

Would need some clear comments on why it exists though!
(+ maybe a better name)


>  	int length;
>  	int ret;
>  

> diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c
> index 0a40f425c27723ccec49985b8b5e14a737b6a7eb..6b7856e69f5fb7b8b73166b9b6825f4af7b19129 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,7 +64,7 @@ static int ingenic_battery_get_property(struct power_supply *psy,
>   */
>  static int ingenic_battery_set_scale(struct ingenic_battery *bat)
>  {
> -	const int *scale_raw;
> +	const int *scale_raw __free(kfree) = NULL;
This isn't a good pattern as per the docs I just replied to v3 with.
Whilst the code is functionally correct today, it is fragile for the reasons
in those docs.

>  	int scale_len, scale_type, best_idx = -1, best_mV, max_raw, i, ret;
>  	u64 max_mV;
>