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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Mar 2020 12:23:34 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Chris Wilson' <chris@...is-wilson.co.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] list: Prevent compiler reloads inside 'safe' list
 iteration

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)