On Tue, Mar 24, 2020 at 05:23:30PM +0100, Marco Elver wrote: > On Tue, 24 Mar 2020 at 16:37, Will Deacon <will@xxxxxxxxxx> wrote: > > > > Some list predicates can be used locklessly even with the non-RCU list > > implementations, since they effectively boil down to a test against > > NULL. For example, checking whether or not a list is empty is safe even > > in the presence of a concurrent, tearing write to the list head pointer. > > Similarly, checking whether or not an hlist node has been hashed is safe > > as well. > > > > Annotate these lockless list predicates with data_race() and READ_ONCE() > > so that KCSAN and the compiler are aware of what's going on. The writer > > side can then avoid having to use WRITE_ONCE() in the non-RCU > > implementation. > > > > Cc: Paul E. McKenney <paulmck@xxxxxxxxxx> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > > Cc: Marco Elver <elver@xxxxxxxxxx> > > Signed-off-by: Will Deacon <will@xxxxxxxxxx> > > --- > > include/linux/list.h | 10 +++++----- > > include/linux/list_bl.h | 5 +++-- > > include/linux/list_nulls.h | 6 +++--- > > include/linux/llist.h | 2 +- > > 4 files changed, 12 insertions(+), 11 deletions(-) > > > > diff --git a/include/linux/list.h b/include/linux/list.h > > index 4fed5a0f9b77..4d9f5f9ed1a8 100644 > > --- a/include/linux/list.h > > +++ b/include/linux/list.h > > @@ -279,7 +279,7 @@ static inline int list_is_last(const struct list_head *list, > > */ > > static inline int list_empty(const struct list_head *head) > > { > > - return READ_ONCE(head->next) == head; > > + return data_race(READ_ONCE(head->next) == head); > > Double-marking should never be necessary, at least if you want to make > KCSAN happy. From what I gather there is an unmarked write somewhere, > correct? In that case, KCSAN will still complain because if it sees a > race between this read and the other write, then at least one is still > plain (the write). > > Then, my suggestion would be to mark the write with data_race() and > just leave this as a READ_ONCE(). Having a data_race() somewhere only > makes KCSAN stop reporting the race if the paired access is also > marked (be it with data_race() or _ONCE, etc.). > > Alternatively, if marking the write is impossible, you can surround > the access with kcsan_disable_current()/kcsan_enable_current(). Or, as > a last resort, just leaving as-is is fine too, because KCSAN's default > config (still) has KCSAN_ASSUME_PLAIN_WRITES_ATOMIC selected. Right, it looks like this is a bit of a smoking gun and we need to decide on whether list_empty() is actually usable without synchronisation first. Based on the outcome of that discussion, I'll update this patch accordingly. The main thing I want to avoid is marking parts of the non-RCU list implementation with data_race() or {READ,WRITE}_ONCE() because that's a sure-fire way to hide real bugs. Cheers, Will