linux-kernel - Re: [PATCH 10/22] gpio: reinforce desc->flags handling

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] [day] [month] [year] [list]

Date: Thu, 1 Feb 2024 19:30:51 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Linus Walleij <linus.walleij@...aro.org>
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 10/22] gpio: reinforce desc->flags handling

On Wed, Jan 31, 2024 at 9:35 PM Linus Walleij <linus.walleij@...aroorg> wrote:
>
> On Wed, Jan 31, 2024 at 9:01 PM Linus Walleij <linus.walleij@...aro.org> wrote:
> > On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@...evpl> wrote:
> >
> > > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > >
> > > We now removed the gpio_lock spinlock and modified the places
> > > previously protected by it to handle desc->flags access in a consistent
> > > way. Let's improve other places that were previously unprotected by
> > > reading the flags field of gpio_desc once and using the stored value for
> > > logic consistency. If we need to modify the field, let's also write it
> > > back once with a consistent value resulting from the function's logic.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > (...)
> >
> > I have a trouble with this one:
> >
> > gpiochip_find_base_unlocked()
> > > +       unsigned long flags;
> > (...)
> > > +       flags = READ_ONCE(desc->flags);
> > (...)
> > > +       if (test_bit(FLAG_OPEN_DRAIN, &flags) &&
> > > +           test_bit(FLAG_IS_OUT, &flags))
> > >                 return 0;
> > (...)
> > > +       assign_bit(FLAG_IS_OUT, &flags, !ret);
> > > +       WRITE_ONCE(desc->flags, flags);
> >
> > I unerstand the atomicity of each operation here, but ... if what you want
> > to protect is modifications from other CPUs, how do we know that another
> > CPU isn't coming in and reading and modifying and assigning
> > another flag inbetween these operations while the value is only
> > stored in the CPU-local flags variable?
> >
> > Same with gpiod_direction_output().
> >
> > To me it seems like maybe you need to actually protect the desc->flags
> > with the SRCU struct in these cases? (and not only use it for the
> > label protection then).
> >
> > An alternative is maybe to rewrite the code with test_and_set().
> >
> > But as you say it is currently unprotected, I just wonder if this really
> > adds any protection.
>
> After re-reading the cover letter I'm fine with this, but I still wonder
> if it buys us anything.
>

This was a tough one...

I don't really see any way around it. SRCU is for pointers but even
then - we wouldn't get with SRCU anything more than what we're getting
with atomic reads and writes. As neither sleeping nor atomic locks
will work in the case of the GPIO subsystem, I figured that we should
strive for the maximum of coherence we can achieve - and for that I
figured that we should read the flags once, do our thing and then
write back a consistent result. If someone else comes around at the
same time and writes something else - well, he better be an
*exclusive*  user of that GPIO and know what they're doing. :)

Anyway, I think this series is already a big step forward and should
at least protect us from crashing. We can continue the work on
achieving full state consistency later.

Bart

> Maybe some words looped back from the
> commit message that we are not really protecting the callbacks
> because access is [predominantly] exclusive?
>
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
>
> Yours,
> Linus Walleij