Re: [PATCH v2 5/7] iio: inkern: copy/release available info from producer

[Matteo Martelli]

Quoting Nuno Sá (2024-10-08 09:29:14)
> On Tue, 2024-10-08 at 08:47 +0200, Matteo Martelli wrote:
> > Quoting Nuno Sá (2024-10-07 17:15:13)
> > > On Mon, 2024-10-07 at 10:37 +0200, Matteo Martelli wrote:
> > > > Consumers need to call the read_avail_release_resource after reading the
> > > > available info. To call the release with info_exists locked, copy the
> > > > available info from the producer and immediately call its release
> > > > callback. With this change, users of iio_read_avail_channel_raw() and
> > > > iio_read_avail_channel_attribute() must free the copied avail info after
> > > > calling them.
> > > > 
> > > > Signed-off-by: Matteo Martelli <matteomartelli3@xxxxxxxxx>
> > > > ---
> > > >  drivers/iio/inkern.c         | 64 +++++++++++++++++++++++++++++++++------
> > > > -----
> > > >  include/linux/iio/consumer.h |  4 +--
> > > >  2 files changed, 50 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > > index
> > > > 7f325b3ed08fae6674245312cf8f57bb151006c0..cc65ef79451e5aa2cea447e168007a44
> > > > 7ffc0d91
> > > > 100644
> > > > --- a/drivers/iio/inkern.c
> > > > +++ b/drivers/iio/inkern.c
> > > > @@ -760,9 +760,25 @@ 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;
> > > > +
> > > > +             *vals = kmemdup_array(vals_tmp, *length, sizeof(int),
> > > > GFP_KERNEL);
> > > > +             if (!*vals)
> > > > +                     return -ENOMEM;
> > > > +
> > > 
> > > Not a big deal but I would likely prefer to avoid yet another copy. If I'm
> > > understanding things correctly, I would rather create an inkern wrapper API
> > > like 
> > > iio_channel_read_avail_release_resource() - maybe something with a smaller
> > > name :).
> > > Hence, the lifetime of the data would be only controlled by the producer of
> > > it. It
> > > would also produce a smaller diff (I think). I just find it a bit confusing
> > > that we
> > > duplicate the data in here and the producer also duplicates it on the -
> > > >read_avail()
> > > call. Another advantage I see is that often the available data is indeed
> > > const in
> > > which case no kmemdup_array() is needed at all.
> > 
> > 
> > If I understand correctly your suggestion you would leave the inkern
> > iio_channel_read_avail() untouched, then add a new inkern wrapper, something
> > like iio_channel_read_avail_release_resource(), that would call the producer's
> > read_avail_release_resource(). The consumer would invoke this new wrapper in
> > its
> > own read_avail_release_resource() avoiding the additional copy. The call stack
> > would look something like the following:
> > 
> > iio_read_channel_info_avail() {
> >     consumer->read_avail() {
> >         iio_read_avail_channel_raw() {
> >             iio_channel_read_avail() {
> >                 producer->read_avail() {
> >                     kmemdup_array();
> >                 }
> >             }
> >         }
> >     }
> > 
> >     iio_format_list();
> > 
> >     consumer->read_avail_release_resource() {
> >         iio_read_avail_channel_release_resource() {
> >             producer->read_avail_release_resource() {
> >                 kfree();
> >             }
> >         }
> >     }
> > }
> 
> Yeah, exactly what came to mind...
> 
> > 
> > 
> > I was going with the simpler solution you described, but my concern with it
> > was
> > that the info_exists_lock mutex would be unlocked between a
> > iio_channel_read_avail()
> > call and its corresponding iio_channel_read_avail_release_resource() call.
> > To my understanding, this could potentially allow for the device to be
> > unregistered between the two calls and result in a memleak of the avail buffer
> > allocated by the producer.
> > 
> > However, I have been trying to reproduce a similar case by adding a delay
> > between the consumer->read_avail() and the
> > consumer->read_avail_release_resources(), and by unbinding the driver during
> > that delay, thus with the info_exists_lock mutex unlocked. In this case the
> > driver is not unregistered until the iio_read_channel_info_avail() function
> > completes, likely because of some other lock on the sysfs file after the call
> > of
> > cdev_device_del() in iio_device_unregister().
> > 
> 
> Yes, you need to have some sync point at the kernfs level otherwise we could
> always be handling a sysfs attr while the device is being removed under our
> feet. But I'm not sure what you're trying to do... IIUC, the problem might come
> if have:
> 
> consumer->read_avail_channel_attribute()
>         producer->info_lock()
>         producer->read_avail()
>                 producer->kmalloc()
> 
> ...
> // producer unbound
> ...
> consumer->read_avail_release()
>         return -ENODEV;
> 
> // producer->kmalloc() never get's freed...
> 
> The above is your problem right? And I think it should be a valid one since
> between ->read_avail_channel_attribute() and read_avail_release() there's
> nothing preventing the producer from being unregistered...

Yes, that's the problem.

> 
> If I'm not missing nothing one solution would be for the producer to do
> devm_kmalloc() and devm_kfree() on read_avail() and release_resources() but at
> that point I'm not sure it's better than what you have since it's odd enough for
> being missed in reviews...

I honestly didn't think of this and it would in fact prevent the
additional copy. But I agree that it could be missed in new drivers,
maybe a comment in the iio_info read_avail_release_resource() callback
declaration would help?

> 
> Anyways, I'm fine with this approach but then I would likely have a comment on
> this extra allocation explaining what is being protected with it as it's not
> straight to realize the subtle race with the producer being gone between calls.
> 
> > Are there are other cases in which the device could be unregistered between
> > the
> > two calls? If the info_exists_lock mutex is not necessary for this
> > read_avail()
> > flow then I could switch it to the simpler solution without the additional
> > consumer
> > copy, but at that point I would question why the info_exists_lock mutex is
> > being
> > locked in iio_read_avail_channel_raw().
> > 
> > For some additional context see also my previous conversation with Jonathan on
> > the subject [1]. I followed Jonathan's suggestion to keep the implementation
> > simple by letting the consumer to always copy the producer buffer, but I could
> > also consider different solutions.
> > 
> > Regarding the release function names being too long, I totally agree and I
> > would also
> > shorten the iio_info read_avail_release_resource() callback if that remains
> > clear: something like read_avail_release_res() or just read_avail_release()?
> > 
> > Link:
> > https://lore.kernel.org/linux-iio/20240810105411.705cb225@jic23-huawei/ [1]
> > 
> 
> Yups, I should have checked v1...

Just to clarify, that link is not the v1 of this patch set but a
previous conversation during the pac1921 driver implementation.

> 
> - Nuno Sá

Thanks,
Matteo Martelli