Re: [PATCH] gpiolib: tie module references to GPIO devices, not requested descs

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Aug 21, 2023 at 12:00 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> On Mon, Aug 21, 2023 at 11:43 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > On Fri, Aug 18, 2023 at 09:01:08PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >
> > > After a deeper look at commit 3386fb86ecde ("gpiolib: fix reference
> > > leaks when removing GPIO chips still in use") I'm now convinced that
> > > gpiolib gets module reference counting wrong.
> > >
> > > As we only take the reference to the owner module when a descriptor is
> > > requested and put it when it's freed, we can easily trigger a crash by
> > > removing a module which registered a driver bound to a GPIO chip which
> > > is unused as nothing prevents us from doing so.
> > >
> > > For correct behavior, we should take the reference to the module when
> > > we're creating a GPIO device and only put it when that device is
> > > released as it's at this point that we can safely remove the module's
> > > code from memory.
> >
> > Two cases to consider:
> > 1) legacy gpio_*() APIs, do they suppose to create a GPIO device?
>
> Legacy uses descriptors under the hood so there must be a GPIO device.
>
> > 2) IRQ request without GPIO being requested, is it the case?
>
> I need to double-check and also test this but it seems to me that
> right now if you do this (request an irq from a GPIO irqchip), the
> reference count of the module will not be increased. With this change
> it will have already been at 1 until the GPIO device backing this irq
> will go down. So it should actually fix another use-after-free bug.
> But don't take my word for it, I will test it later when I have the
> time.
>
> There's another issue that will become visible with this patch -
> namely the modules that register devices from their init functions,
> will no longer allow unloading until the device is unbound first. This
> is not wrong wrong as module's init is not the place to register
> devices, platform or otherwise but I'm wondering if it counts as
> breaking someone's setup?
>
> Bart
>

Ok so just checked in theory and verified in practice: with an irq
request orthogonal to the GPIO descriptor, when the GPIO device goes
down, it destroys the irq domain (side note: gpio-sim now finally
disposes of all existing mappings too which would have been the source
of an error here). When the user calls free_irq(), the underlying
irq_do_desc() calls mtree_load() which now returns NULL (mapping is
gone) and nothing happens.

This change doesn't change that behavior - you can still unbind the
GPIO device at any moment and the irq user will be fine.

The problem is: I can no longer reproduce the crash I saw in KASAN
with current next and I'm thinking I may have mistaken one of the bugs
I recently fixed for the culprit here. What I'm seeing now when a
module is unloaded is: driver gets unregistered, device gets unbound
and that's it, all works fine. So this patch and the libgpiod one may
have been pointless noise. :(

Taking the module reference only when there's a requested descriptor
is in line with what most other frameworks do as well.

I need more coffee but maybe at this point I should switch to
panzerschokolade...

Bart