Re: [GIT PULL] gpio: fixes for v5.18-rc2

[Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>]

On Sat, Apr 9, 2022 at 10:51 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> Here's a single fix for a race condition between the GPIO core and consumers of
> GPIO IRQ chips.

I've pulled this, but it's horribly broken.

You can't just use a compiler barrier to make sure the compiler orders
the data at initialization time.

That doesn't take care of CPU re-ordering, but it also doesn't take
care of re-ordering reads on the other side of the equation.

Every write barrier needs to pair with a read barrier.

And "barrier()" is only a barrier on that CPU, since it is only a
barrier for code generation, not for data.

There are multiple ways to do proper hand-off of data, but the best
one is likely

 - on the initialization side, do

        .. initialize all the data, then do ..
        smp_store_release(&initialized, 1);

 - on the reading side, do

        if (!smp_load_acquire(&initialized))
                 return -EAGAIN;

        .. you can now rely on all the data having been initialized ..

But honestly, the fact that you got this race condition so wrong makes
me suggest you use proper locks. Because the above gives you proper
ordering between the two sequences, but the sequences in question
still have to have a *lot* of guarantees about the accesses actually
then being valid in a lock-free environment (the only obviously safe
situation is a "initialize things once, everything afterwards is only
a read" - otherwise y ou need to make sure all the *updates* are
safely done too).

With locking, all these issues go away. The lock will take care of
ordering, but also data consistency at updates.

Without locking, you need to do the above kinds of careful things for
_all_ the accesses that can race, not just that "initialized" flag.

                 Linus