RE: [PATCH] list: Prevent compiler reloads inside 'safe' list iteration

[Chris Wilson]

Quoting David Laight (2020-03-10 12:23:34)
> From: Chris Wilson
> > Sent: 10 March 2020 11:50
> > 
> > Quoting David Laight (2020-03-10 11:36:41)
> > > From: Chris Wilson
> > > > Sent: 10 March 2020 09:21
> > > > 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).
> > >
> > > Eh?
> > > I thought any function call will stop the compiler being allowed
> > > to reload the value.
> > > The 'safe' loop iterators are only 'safe' against called
> > > code removing the current item from the list.
> > >
> > > > This helps prevent kcsan warnings over 'unsafe' conduct in releasing the
> > > > list elements during list_for_each_entry_safe() and friends.
> > >
> > > Sounds like kcsan is buggy ????
> > 
> > The warning kcsan gave made sense (a strange case where the emptying the
> > list from inside the safe iterator would allow that list to be taken
> > under a global mutex and have one extra request added to it. The
> > list_for_each_entry_safe() should be ok in this scenario, so long as the
> > next element is read before this element is dropped, and the compiler is
> > instructed not to reload the element.
> 
> Normally the loop iteration code has to hold the mutex.
> I guess it can be released inside the loop provided no other
> code can ever delete entries.
> 
> > kcsan is a little more insistent on having that annotation :)
> > 
> > In this instance I would say it was a false positive from kcsan, but I
> > can see why it would complain and suspect that given a sufficiently
> > aggressive compiler, we may be caught out by a late reload of the next
> > element.
> 
> If you have:
>         for (; p; p = next) {
>                 next = p->next;
>                 external_function_call(void);
>         }
> the compiler must assume that the function call
> can change 'p->next' and read it before the call.
> 
> Is this a list with strange locking rules?

Yes.

> The only deletes are from within the loop.

All deletes are within the mutex.

> Adds and deletes are locked.

There's just one special case where after the very last element of all
lists for an engine is removed, a global mutex is taken and one new
element is added to one of the lists to track powering off the engine.

> The list traversal isn't locked.

There's rcu traversal of the list as well.
 
> I suspect kcsan bleats because it doesn't assume the compiler
> will use a single instruction/memory operation to read p->next.
> That is just stupid.

kcsan is looking for a write to a pointer after a read that is not in
the same locking chain. While I have satisfied lockdep that I am not
insane, I'm worrying in case kcsan has a valid objection to the
potential data race in the safe list iterator.
-Chris