| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=McPQnLzPCJz6AKV44=qp6z=V9sCAcYkDTQA6FVjWj6E3A@mail.gmail.com>
Date: Tue, 19 Dec 2023 10:30:07 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Kent Gibson <warthog618@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linus.walleij@...aro.org, andy@...nel.org
Subject: Re: [PATCH v5 3/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()
On Tue, Dec 19, 2023 at 1:42 AM Kent Gibson <warthog618@...il.com> wrote:
>
> Reduce the time holding the gpio_lock by snapshotting the desc flags,
> rather than testing them individually while holding the lock.
>
> Accept that the calculation of the used field is inherently racy, and
> only check the availability of the line from pinctrl if other checks
> pass, so avoiding the check for lines that are otherwise in use.
>
> Signed-off-by: Kent Gibson <warthog618@...il.com>
> Reviewed-by: Andy Shevchenko <andy@...nel.org>
> ---
> drivers/gpio/gpiolib-cdev.c | 74 ++++++++++++++++++-------------------
> 1 file changed, 36 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index aecc4241b6c8..9f5104d7532f 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2399,74 +2399,72 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> struct gpio_v2_line_info *info)
> {
> struct gpio_chip *gc = desc->gdev->chip;
> - bool ok_for_pinctrl;
> - unsigned long flags;
> + unsigned long dflags;
>
> memset(info, 0, sizeof(*info));
> info->offset = gpio_chip_hwgpio(desc);
>
> - /*
> - * 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(gc, info->offset);
> + scoped_guard(spinlock_irqsave, &gpio_lock) {
> + if (desc->name)
> + strscpy(info->name, desc->name, sizeof(info->name));
>
> - spin_lock_irqsave(&gpio_lock, flags);
> + if (desc->label)
> + strscpy(info->consumer, desc->label,
> + sizeof(info->consumer));
>
> - if (desc->name)
> - strscpy(info->name, desc->name, sizeof(info->name));
> -
> - if (desc->label)
> - strscpy(info->consumer, desc->label, sizeof(info->consumer));
> + dflags = READ_ONCE(desc->flags);
> + }
>
> /*
> - * Userspace only need to know that the kernel is using this GPIO so
> - * it can't use it.
> + * Userspace only need know that the kernel is using this GPIO so it
> + * can't use it.
> + * The calculation of the used flag is slightly racy, as it may read
> + * desc, gc and pinctrl state without a lock covering all three at
> + * once. Worst case if the line is in transition and the calculation
> + * is inconsistent then it looks to the user like they performed the
> + * read on the other side of the transition - but that can always
> + * happen.
> + * The definitive test that a line is available to userspace is to
> + * request it.
> */
> - info->flags = 0;
> - if (test_bit(FLAG_REQUESTED, &desc->flags) ||
> - test_bit(FLAG_IS_HOGGED, &desc->flags) ||
> - test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
> - test_bit(FLAG_EXPORT, &desc->flags) ||
> - test_bit(FLAG_SYSFS, &desc->flags) ||
> + if (test_bit(FLAG_REQUESTED, &dflags) ||
> + test_bit(FLAG_IS_HOGGED, &dflags) ||
> + test_bit(FLAG_USED_AS_IRQ, &dflags) ||
> + test_bit(FLAG_EXPORT, &dflags) ||
> + test_bit(FLAG_SYSFS, &dflags) ||
> !gpiochip_line_is_valid(gc, info->offset) ||
> - !ok_for_pinctrl)
> + !pinctrl_gpio_can_use_line(gc, info->offset))
> info->flags |= GPIO_V2_LINE_FLAG_USED;
>
> - if (test_bit(FLAG_IS_OUT, &desc->flags))
> + if (test_bit(FLAG_IS_OUT, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_OUTPUT;
> else
> info->flags |= GPIO_V2_LINE_FLAG_INPUT;
>
> - if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> + if (test_bit(FLAG_ACTIVE_LOW, &dflags))
One more nit: you no longer have to use atomic bitops here, you can
switch to the ones without guarantees for better performance.
Bart
> info->flags |= GPIO_V2_LINE_FLAG_ACTIVE_LOW;
>
> - if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
> + if (test_bit(FLAG_OPEN_DRAIN, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_OPEN_DRAIN;
> - if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
> + if (test_bit(FLAG_OPEN_SOURCE, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_OPEN_SOURCE;
>
> - if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
> + if (test_bit(FLAG_BIAS_DISABLE, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_BIAS_DISABLED;
> - if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> + if (test_bit(FLAG_PULL_DOWN, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
> - if (test_bit(FLAG_PULL_UP, &desc->flags))
> + if (test_bit(FLAG_PULL_UP, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
>
> - if (test_bit(FLAG_EDGE_RISING, &desc->flags))
> + if (test_bit(FLAG_EDGE_RISING, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_EDGE_RISING;
> - if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
> + if (test_bit(FLAG_EDGE_FALLING, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING;
>
> - if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags))
> + if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME;
> - else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
> + else if (test_bit(FLAG_EVENT_CLOCK_HTE, &dflags))
> info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
> -
> - spin_unlock_irqrestore(&gpio_lock, flags);
> }
>
> struct gpio_chardev_data {
> --
> 2.39.2
>
Powered by blists - more mailing lists