Message-ID: <8241b4caf9ebacb116f50bfe1503f325cc576066.camel@gmail.com>
Date: Tue, 08 Oct 2024 09:29:14 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Matteo Martelli <matteomartelli3@...il.com>, Alisa-Dariana Roman	
 <alisa.roman@...log.com>, Christian Eggers <ceggers@...i.de>, Jonathan
 Cameron	 <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich	 <Michael.Hennerich@...log.com>, Paul Cercueil
 <paul@...pouillou.net>, Peter Rosin <peda@...ntia.se>, Sebastian Reichel
 <sre@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mips@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 5/7] iio: inkern: copy/release available info from
 producer

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@...il.com>
> > > ---
> > >  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á
> 

Powered by blists - more mailing lists