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

[David Laight]

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?
The only deletes are from within the loop.
Adds and deletes are locked.
The list traversal isn't locked.

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.

	David



		

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)