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:50:08 +0000
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        David Laight <David.Laight@...LAB.COM>
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

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