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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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

>
> Seems to me that the 1) is the case, while 2) is not.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>