linux-kernel - Re: [PATCH 09/22] gpio: remove gpio

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdY8v2x6VzRnEV6baa4_XLGLm0BOe6Nj6wzjujHxqEDHVA@mail.gmail.com>
Date: Wed, 31 Jan 2024 20:51:14 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Kent Gibson <warthog618@...il.com>, Alex Elder <elder@...aro.org>, 
	Geert Uytterhoeven <geert+renesas@...der.be>, "Paul E . McKenney" <paulmck@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Wolfram Sang <wsa@...-dreams.de>, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 09/22] gpio: remove gpio_lock

On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> The "multi-function" gpio_lock is pretty much useless with how it's used
> in GPIOLIB currently. Because many GPIO API calls can be called from all
> contexts but may also call into sleeping driver callbacks, there are
> many places with utterly broken workarounds like yielding the lock to
> call a possibly sleeping function and then re-acquiring it again without
> taking into account that the protected state may have changed.
>
> It was also used to protect several unrelated things: like individual
> descriptors AND the GPIO device list. We now serialize access to these
> two with SRCU and so can finally remove the spinlock.
>
> There is of course the question of consistency of lockless access to
> GPIO descriptors. Because we only support exclusive access to GPIOs
> (officially anyway, I'm looking at you broken
> GPIOD_FLAGS_BIT_NONEXCLUSIVE bit...) and the API contract with providers
> does not guarantee serialization, it's enough to ensure we cannot
> accidentally dereference an invalid pointer and that the state we present
> to both users and providers remains consistent. To achieve that: read the
> flags field atomically except for a few special cases. Read their current
> value before executing callback code and use this value for any subsequent
> logic. Modifying the flags depends on the particular use-case and can
> differ. For instance: when requesting a GPIO, we need to set the
> REQUESTED bit immediately so that the next user trying to request the
> same line sees -EBUSY.
>
> While at it: the allocations that used GFP_ATOMIC until this point can
> now switch to GFP_KERNEL.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>

Neat!
Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

(I'm sorry about NONEXCLUSIVE, let's see what we can do about it...)

> @@ -578,6 +577,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>                 return -EINVAL;
>         }
>
> +       if (!test_and_set_bit(FLAG_EXPORT, &desc->flags))
> +               return -EPERM;

This exit early split off from the big if() below (which is good) is
a new thing right? Maybe mention this in the commit message?

> -/* gpio_lock prevents conflicts during gpio_desc[] table updates.
> - * While any GPIO is requested, its gpio_chip is not removable;
> - * each GPIO's "requested" flag serves as a lock and refcount.
> - */
> -DEFINE_SPINLOCK(gpio_lock);

GOOD RIDDANCE.

> -               /* FIXME: make this GFP_KERNEL once the spinlock is out. */
> -               new = kstrdup_const(label, GFP_ATOMIC);
> +               new = kstrdup_const(label, GFP_KERNEL);

And all of this is neat as well.

Someone might complain about splitting that in a separate patch,
but not me because I don't care so much about such processy
things. (The patchset is already split enough as it is.)

Yours,
Linus Walleij