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...
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...
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...
- Nuno Sá
>
