[PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
as it takes a mutex internally. Let's move the call before taking the
spinlock and store the return value.

This isn't perfect - there's a moment between calling
pinctrl_gpio_can_use_line() and taking the spinlock where the situation
can change but it isn't a regression either: previously this part wasn't
protected at all and it only affects the information user-space is
seeing.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 02f8b2b81199..ee20634a522c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1154,8 +1154,19 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 				  struct gpioline_info *info)
 {
 	struct gpio_chip *chip = desc->gdev->chip;
+	bool ok_for_pinctrl;
 	unsigned long flags;
 
+	/*
+	 * This function takes a mutex so we must check this before taking
+	 * the spinlock.
+	 *
+	 * FIXME: find a non-racy way to retrieve this information. Maybe a
+	 * lock common to both frameworks?
+	 */
+	ok_for_pinctrl = pinctrl_gpio_can_use_line(
+				chip->base + info->line_offset);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	if (desc->name) {
@@ -1182,7 +1193,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	    test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
 	    test_bit(FLAG_EXPORT, &desc->flags) ||
 	    test_bit(FLAG_SYSFS, &desc->flags) ||
-	    !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
+	    !ok_for_pinctrl)
 		info->flags |= GPIOLINE_FLAG_KERNEL;
 	if (test_bit(FLAG_IS_OUT, &desc->flags))
 		info->flags |= GPIOLINE_FLAG_IS_OUT;
-- 
2.25.0

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Geert Uytterhoeven]

Hi Bartosz,

On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> as it takes a mutex internally. Let's move the call before taking the
> spinlock and store the return value.
>
> This isn't perfect - there's a moment between calling
> pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> can change but it isn't a regression either: previously this part wasn't
> protected at all and it only affects the information user-space is
> seeing.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Thanks for your patch!

> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1154,8 +1154,19 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>                                   struct gpioline_info *info)
>  {
>         struct gpio_chip *chip = desc->gdev->chip;
> +       bool ok_for_pinctrl;
>         unsigned long flags;
>
> +       /*
> +        * This function takes a mutex so we must check this before taking
> +        * the spinlock.
> +        *
> +        * FIXME: find a non-racy way to retrieve this information. Maybe a
> +        * lock common to both frameworks?
> +        */
> +       ok_for_pinctrl = pinctrl_gpio_can_use_line(
> +                               chip->base + info->line_offset);

Note that this is now called unconditionally, while before it was only called
if all previous steps in the ||-pipeline failed.

> +
>         spin_lock_irqsave(&gpio_lock, flags);
>
>         if (desc->name) {
> @@ -1182,7 +1193,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>             test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
>             test_bit(FLAG_EXPORT, &desc->flags) ||
>             test_bit(FLAG_SYSFS, &desc->flags) ||
> -           !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
> +           !ok_for_pinctrl)
>                 info->flags |= GPIOLINE_FLAG_KERNEL;
>         if (test_bit(FLAG_IS_OUT, &desc->flags))
>                 info->flags |= GPIOLINE_FLAG_IS_OUT;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Bartosz Golaszewski]

pt., 20 mar 2020 o 11:59 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> Hi Bartosz,
>
> On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> > as it takes a mutex internally. Let's move the call before taking the
> > spinlock and store the return value.
> >
> > This isn't perfect - there's a moment between calling
> > pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> > can change but it isn't a regression either: previously this part wasn't
> > protected at all and it only affects the information user-space is
> > seeing.
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Thanks for your patch!
>
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1154,8 +1154,19 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> >                                   struct gpioline_info *info)
> >  {
> >         struct gpio_chip *chip = desc->gdev->chip;
> > +       bool ok_for_pinctrl;
> >         unsigned long flags;
> >
> > +       /*
> > +        * This function takes a mutex so we must check this before taking
> > +        * the spinlock.
> > +        *
> > +        * FIXME: find a non-racy way to retrieve this information. Maybe a
> > +        * lock common to both frameworks?
> > +        */
> > +       ok_for_pinctrl = pinctrl_gpio_can_use_line(
> > +                               chip->base + info->line_offset);
>
> Note that this is now called unconditionally, while before it was only called
> if all previous steps in the ||-pipeline failed.
>

Is this a problem though? Doesn't seem so. Am I missing something?

Bart

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Linus Walleij]

On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> as it takes a mutex internally. Let's move the call before taking the
> spinlock and store the return value.
>
> This isn't perfect - there's a moment between calling
> pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> can change but it isn't a regression either: previously this part wasn't
> protected at all and it only affects the information user-space is
> seeing.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I'm sorry that I lost track of this patch :(

Do we still need something like this or has it been fixed
by some other patches?

Yours,
Linus Walleij

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Bartosz Golaszewski]

wt., 14 kwi 2020 o 14:00 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> > as it takes a mutex internally. Let's move the call before taking the
> > spinlock and store the return value.
> >
> > This isn't perfect - there's a moment between calling
> > pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> > can change but it isn't a regression either: previously this part wasn't
> > protected at all and it only affects the information user-space is
> > seeing.
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> I'm sorry that I lost track of this patch :(
>
> Do we still need something like this or has it been fixed
> by some other patches?
>
> Yours,
> Linus Walleij

Nope, this is still an issue. Do you have a better idea than mine?

Bart

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Linus Walleij]

On Tue, Apr 14, 2020 at 2:27 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> wt., 14 kwi 2020 o 14:00 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> > > as it takes a mutex internally. Let's move the call before taking the
> > > spinlock and store the return value.
> > >
> > > This isn't perfect - there's a moment between calling
> > > pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> > > can change but it isn't a regression either: previously this part wasn't
> > > protected at all and it only affects the information user-space is
> > > seeing.
> > >
> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Fixes: d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > I'm sorry that I lost track of this patch :(
> >
> > Do we still need something like this or has it been fixed
> > by some other patches?
> >
> > Yours,
> > Linus Walleij
>
> Nope, this is still an issue. Do you have a better idea than mine?

Nope, can you just queue it in your tree?
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Andy Shevchenko]

On Tue, Apr 14, 2020 at 6:35 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> wt., 14 kwi 2020 o 14:00 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> > > as it takes a mutex internally. Let's move the call before taking the
> > > spinlock and store the return value.
> > >
> > > This isn't perfect - there's a moment between calling
> > > pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> > > can change but it isn't a regression either: previously this part wasn't
> > > protected at all and it only affects the information user-space is
> > > seeing.

It seems I have no original at hand, so, commenting here.

It looks like we need a mutex less function which can be used here and
in the call you are considering racy.
Note, mutex followed by spin lock is fine, other way around is not.

So, here you should have something like

mutex_lock
ok_for_gpio = ...
spin_lock
...
spin_unlock
mutex_unlock.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Bartosz Golaszewski]

wt., 28 kwi 2020 o 17:53 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
>
> On Tue, Apr 14, 2020 at 6:35 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > wt., 14 kwi 2020 o 14:00 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > >
> > > On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > We must not call pinctrl_gpio_can_use_line() with the gpio_lock taken
> > > > as it takes a mutex internally. Let's move the call before taking the
> > > > spinlock and store the return value.
> > > >
> > > > This isn't perfect - there's a moment between calling
> > > > pinctrl_gpio_can_use_line() and taking the spinlock where the situation
> > > > can change but it isn't a regression either: previously this part wasn't
> > > > protected at all and it only affects the information user-space is
> > > > seeing.
>
> It seems I have no original at hand, so, commenting here.
>
> It looks like we need a mutex less function which can be used here and
> in the call you are considering racy.

The thing is this mutex is in pinctrl - we'd need to export it too so
that gpio can use it.

Bart

Re: [PATCH] gpiolib: don't call sleeping functions with a spinlock taken

[Andy Shevchenko]

On Wed, Apr 29, 2020 at 9:40 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> wt., 28 kwi 2020 o 17:53 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a):
> > On Tue, Apr 14, 2020 at 6:35 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > > wt., 14 kwi 2020 o 14:00 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > > > On Fri, Mar 20, 2020 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> > It looks like we need a mutex less function which can be used here and
> > in the call you are considering racy.
>
> The thing is this mutex is in pinctrl - we'd need to export it too so
> that gpio can use it.

Oh, I see now. So, something like

pinctrl_ext_operation_begin()
....
pinctrl_ext_operation_end()

perhaps.

-- 
With Best Regards,
Andy Shevchenko