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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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.

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.

-- 
With Best Regards,
Andy Shevchenko