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]

Message-ID: <20200310125031.GY2935@paulmck-ThinkPad-P72>
Date:   Tue, 10 Mar 2020 05:50:31 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Chris Wilson' <chris@...is-wilson.co.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>, elver@...gle.com
Subject: Re: [PATCH] list: Prevent compiler reloads inside 'safe' list
 iteration

On Tue, Mar 10, 2020 at 12:23:34PM +0000, David Laight wrote:
> 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 ????

Adding Marco on CC for his thoughts.

> > 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.

That "must assume" is a statement of current compiler technology.
Given the progress over the past forty years, I would not expect this
restriction to hold forever.  Yes, we can and probably will get the
compiler implementers to give us command-line flags to suppress global
analysis.  But given the progress in compilers that I have seen over
the past 4+ decades, I would expect that the day will come when we won't
want to be using those command-line flags.

But if you want to ignore KCSAN's warnings, you are free to do so.

> 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.

Heh!  If I am still around, I will ask you for your evaluation of the
above statement in 40 years.  Actually, 10 years will likely suffice.  ;-)

							Thanx, Paul