Re: [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup

[Bartosz Golaszewski]

On Thu, Dec 21, 2023 at 7:43 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> There's time between when we locate the relevant descriptor during
> lookup and when we actually take the reference to its parent GPIO
> device where - if the GPIO device in question is removed - we'll end up
> with a dangling pointer to freed memory. Make sure devices cannot be
> removed until we hold a new reference to the device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>  drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 1baeb6778ec6..8a15b8f6b50e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4147,27 +4147,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>         struct gpio_desc *desc;
>         int ret;
>
> -       desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
> -       if (gpiod_not_found(desc) && platform_lookup_allowed) {
> +       scoped_guard(rwsem_read, &gpio_devices_sem) {

I am too sleep deprived to be coding. This doesn't make sense because
if we are held at the write semaphore in gpiodev_release() then it's
already too late as the device is being removed already and we'll
still end up with a dangling pointer.

Should we hold off any reference putting for all GPIO devices when a
descriptor lookup is in progress?

Bart

> +               desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> +                                           &flags, &lookupflags);
> +               if (gpiod_not_found(desc) && platform_lookup_allowed) {
> +                       /*
> +                        * Either we are not using DT or ACPI, or their lookup
> +                        * did not return a result. In that case, use platform
> +                        * lookup as a fallback.
> +                        */
> +                       dev_dbg(consumer,
> +                               "using lookup tables for GPIO lookup\n");
> +                       desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> +               }
> +
> +               if (IS_ERR(desc)) {
> +                       dev_dbg(consumer, "No GPIO consumer %s found\n",
> +                               con_id);
> +                       return desc;
> +               }
> +
>                 /*
> -                * Either we are not using DT or ACPI, or their lookup did not
> -                * return a result. In that case, use platform lookup as a
> -                * fallback.
> +                * If a connection label was passed use that, else attempt to
> +                * use the device name as label
>                  */
> -               dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> -               desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> +               ret = gpiod_request(desc, label);
>         }
>
> -       if (IS_ERR(desc)) {
> -               dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> -               return desc;
> -       }
> -
> -       /*
> -        * If a connection label was passed use that, else attempt to use
> -        * the device name as label
> -        */
> -       ret = gpiod_request(desc, label);
>         if (ret) {
>                 if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
>                         return ERR_PTR(ret);
> --
> 2.40.1
>