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

PATCH In review Bartosz Golaszewski 5 participants 9 messages Mar 20, 2020 – Apr 29, 2020 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2020-03-20 09:31 UTC #0 lore raw

PATCH 1/1 gpiolib: don't call sleeping functions with a spinlock taken

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

Fixes d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/gpio/gpiolib.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/gpio/gpiolib.c +12 -1

index 02f8b2b81199..ee20634a522c 100644
@@ -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;

Signature · 1 line

2.25.0

Geert Uytterhoeven

2020-03-20 10:59 UTC #1 lore raw

Hi Bartosz,

Quoted 12 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 3 lines

Reported-by Geert Uytterhoeven <[email protected]>
Fixes d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
Signed-off-by Bartosz Golaszewski <[email protected]>

Thanks for your patch!

Quoted 8 lines

drivers/gpio/gpiolib.c +1 -0

@@ -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;

drivers/gpio/gpiolib.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1168

Quoted 10 lines

drivers/gpio/gpiolib.c +9 -0

 
+       /*
+        * 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.

drivers/gpio/gpiolib.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1199

Quoted 13 lines

drivers/gpio/gpiolib.c +2 -1

+
        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

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Bartosz Golaszewski

author

2020-03-20 16:55 UTC #2 lore raw

pt., 20 mar 2020 o 11:59 Geert Uytterhoeven <[email protected]> napisał(a):

Quoted 31 lines

Hi Bartosz,
Quoted 12 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 3 lines
Reported-by Geert Uytterhoeven <[email protected]>
Fixes d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
Signed-off-by Bartosz Golaszewski <[email protected]>
Thanks for your patch!
Quoted 9 lines
drivers/gpio/gpiolib.c +1 -0
@@ -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;
 

Quoted 13 lines

drivers/gpio/gpiolib.c static void gpio_desc_to_lineinfo(struct gpio_desc *desc, · line 1168
Quoted 9 lines
drivers/gpio/gpiolib.c +9 -0
+       /*
+        * 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

Linus Walleij

2020-04-14 12:00 UTC #3 lore raw

In reply to Bartosz Golaszewski

Quoted 12 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 3 lines

Reported-by Geert Uytterhoeven <[email protected]>
Fixes d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
Signed-off-by Bartosz Golaszewski <[email protected]>

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

Bartosz Golaszewski

2020-04-14 12:27 UTC #4 lore raw

wt., 14 kwi 2020 o 14:00 Linus Walleij <[email protected]> napisał(a):

Quoted 19 lines · Bartosz Golaszewski

Quoted 13 lines
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 3 lines
Reported-by Geert Uytterhoeven <[email protected]>
Fixes d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
Signed-off-by Bartosz Golaszewski <[email protected]>

Quoted 7 lines

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

Linus Walleij

Acked-by

2020-04-16 11:21 UTC #5 lore raw

Quoted 22 lines · Bartosz Golaszewski

wt., 14 kwi 2020 o 14:00 Linus Walleij <[email protected]> napisał(a):
Quoted 21 lines · Bartosz Golaszewski
Quoted 13 lines
From: Bartosz Golaszewski <[email protected]>
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.
Quoted 3 lines
Reported-by Geert Uytterhoeven <[email protected]>
Fixes d2ac25798208 ("gpiolib: provide a dedicated function for setting lineinfo")
Signed-off-by Bartosz Golaszewski <[email protected]>
I'm sorry that I lost track of this patch :(

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

2020-04-28 15:53 UTC #6 lore raw

In reply to Bartosz Golaszewski

Quoted 16 lines · Bartosz Golaszewski

wt., 14 kwi 2020 o 14:00 Linus Walleij <[email protected]> napisał(a):
Quoted 14 lines · Bartosz Golaszewski
Quoted 12 lines
From: Bartosz Golaszewski <[email protected]>
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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

2020-04-29 06:40 UTC #7 lore raw

wt., 28 kwi 2020 o 17:53 Andy Shevchenko <[email protected]> napisał(a):

Quoted 20 lines

Quoted 16 lines · Bartosz Golaszewski
wt., 14 kwi 2020 o 14:00 Linus Walleij <[email protected]> napisał(a):
Quoted 14 lines · Bartosz Golaszewski
Quoted 12 lines
From: Bartosz Golaszewski <[email protected]>
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.

Quoted 4 lines

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

Andy Shevchenko

2020-04-29 12:37 UTC #8 lore raw

Quoted 5 lines · Bartosz Golaszewski

wt., 28 kwi 2020 o 17:53 Andy Shevchenko <[email protected]> napisał(a):
Quoted 4 lines
Quoted 2 lines · Bartosz Golaszewski
wt., 14 kwi 2020 o 14:00 Linus Walleij <[email protected]> napisał(a):
Quoted 1 line · Bartosz Golaszewski

...

Quoted 5 lines

Quoted 2 lines
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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko