linux-kernel - Re: [GIT PULL] gpio: fixes for v6.1-rc8

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]

Message-ID: <CAHk-=wi+7kAEK9f+t=z5B39bASzswNCVE+ZJKnDPjAHM_FoRNg@mail.gmail.com>
Date:   Sun, 4 Dec 2022 13:24:00 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [GIT PULL] gpio: fixes for v6.1-rc8 - take 2

On Sun, Dec 4, 2022 at 12:47 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:
>
> No, the data can't be removed with these locks in place. It's just to
> avoid going into the callbacks if gpiochip_remove() is already in
> progress (the only reason why trylock would fail).

That "the only reason why trylock would fail" may be true in practice,
but it's really just an implementation detail.

Other issues *could* make a trylock fail.

For example, assuming the trylock is just implemented as a non-looping
"cmpxchg" (which may or may not be the case), even another reader
coming in and racing with a trylock could make the cmpxchg fail.

That "do one single cmpxchg" is what the spinlock trylock code does,
for example.

Now, that's not actually what we do for the down_read_trylock() case -
we will actually loop over it - but locking is complicated and you
absolutely should not make assumptions about the exact implementation
details.

And even with the loop, if you ever have *any* other reason to get the
write-lock (or even just do a "try_write", suddenly the details of
when the trylock fails changes entirely.

So that's why I really don't want some random driver layer to depend
on some semantics for trylock that aren't actually guaranteed in the
bigger picture.

The only thing "trylock" says is "I tried to get the lock". It really
could easily fail for random reasons that aren't about actual writers.

For example, even aside from any "do a single cmpxchg" issue: a
sleeping lock could be implemented using a spinlock to protect the
"real" locking data. That kind of implementation is particularly
likely for debugging.

And then, in order to be usable in a recursive environment, a
"trylock" would quite likely be implemented without spinning on that
spinlock, even before it gets to the actual lock internal
implementation. So just contention on the spinlock - by other readers,
not writers - could make the trylock fail.

See?

             Linus