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