Re: [patch 125/166] lib/list: prevent compiler reloads inside 'safe' list iteration

Paul E. McKenney
2020-04-08T20:26:33Z

On Tue, Apr 07, 2020 at 08:44:35AM -0700, Linus Torvalds wrote:
> On Mon, Apr 6, 2020 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Subject: lib/list: prevent compiler reloads inside 'safe' list iteration
> >
> > Instruct the compiler to read the next element in the list iteration
> > once, and that it is not allowed to reload the value from the stale
> > element later. This is important as during the course of the safe
> > iteration, the stale element may be poisoned (unbeknownst to the
> > compiler).
>
> Andrew, Chris, this one looks rather questionable to me.
>
> How the heck would the ->next pointer be changed without the compiler
> being aware of it? That implies a bug to begin with - possibly an
> inline asm that changes kernel memory without having a memory clobber.
>
> Quite fundamentally, the READ_ONCE() doesn't seem to fix anything. If
> something else can change the list _concurrently_, it's still
> completely broken, and hiding the KASAN report is just hiding a bug.
>
> What and where was the actual KASAN issue? The commit log doesn't say...

OK, it sounds like we need to specify what needs to be present in this
sort of commit log. Please accept my apologies for this specification
not already being in place.

How about the following?

o The KCSAN splat, optionally including file/line numbers.

o Any non-default Kconfig options that are required to reproduce
the problem, along with any other repeat-by information.

o Detailed description of the problem this splat identifies, for
example, the code might fail to acquire a necessary lock, a plain
load might vulnerable to compiler optimizations, and so on.

o If available, references to or excerpts from the comments
and documentation defining the design rules that the old code
violates.

o If the commit's effect is to silence the warning with no other
algorithmic change, an explanation as to why this is the right
thing to do. Continuing the plain-load example above, that
load might be controlling a loop and the compiler might choose
to hoist the load out of the loop, potentially resulting in an
infinite loop.

Another example might be diagnostic code where the accesses are
not really part of the concurrency design, but where we need
KCSAN checking the other code that implements that design.

Thoughts? Additions? Subtractions?

Thanx, Paul