linux-kernel - Re: [PATCH] futex: replace bare barrier() with a READ

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] [day] [month] [year] [list]

Date:	Wed, 2 Mar 2016 15:12:51 +0800
From:	Jianyu Zhan <nasa4836@...il.com>
To:	Darren Hart <dvhart@...radead.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>, dave@...olabs.net,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>, dvhart@...ux.intel.com,
	bigeasy@...utronix.de, Paul McKenney <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] futex: replace bare barrier() with a READ_ONCE()

On Wed, Mar 2, 2016 at 2:39 PM, Darren Hart <dvhart@...radead.org> wrote:
> I believe you are correct with respect to the retry and while condition being an
> appropriate place for the application of READ_ONCE. The question is why is this
> preferred to the existing barrier()? I suggest:
>
> While barrier() is a fairly brute force general application of a compiler
> barrier, READ_ONCE() is very specific, it targets only operations dealing with
> the specified variable. As such, its application both clearly identifies the
> volatile variable and frees the compiler to make optimizations a more general
> barrier would forbid.
>
>

Yep,  beside the informative point,  the more specifics of READ_ONCE
over barrier
is what I meant  "lightweight", I missed emphasizing this point.
Thanks for pointing it.

I will respin this patch to reflect this.

>>
>>
>> > As for #2...
>> >
>> >> 2.  For the second problem I memtioned,  yes, it is theoretical,  and
>> >> it is also due to  q->lock_ptr can change between
>> >> the test of nullness of q->lock_ptr and the lock on q->lock_ptr.
>> >>
>> >> the code is
>> >>
>> >> retry:
>> >>        lock_ptr = q->lock_ptr;
>> >>        if (lock_ptr != 0)  {
>> >>                   spin_lock(lock_ptr)
>> >>                   if (unlikely(lock_ptr != q->lock_ptr)) {
>> >>                         spin_unlock(lock_ptr);
>> >>                          goto retry;
>> >>                   }
>> >>                    ...
>> >>        }
>> >>
>> >> which is effectively the same as :
>> >>
>> >>  while (lock_ptr = q->lock_ptr) {
>> >>                   spin_lock(lock_ptr)
>> >>                   if (unlikely(lock_ptr != q->lock_ptr)) {
>> >>                         spin_unlock(lock_ptr);
>> >>                          goto retry;
>> >>                   }
>> >>                    ...
>> >> }
>> >>
>> >> This might cause the compiler load the q->lock_ptr once and use it
>> >> many times,  quoted from
>
> Which is already covered by the barrier() in place today as a more general
> compiler barrier.
>
> Your argument is then simply that READ_ONCE is a more specific solution to the
> problem.
>

Yep. And after re-thinking, I am now less convinced in this second
argument, since
it involves a comparison of q->lock_ptr in the loop body,  so this may
defeat any attempts
that compilers try to optimize the load out of the loop,  even without
a READ_ONCE().

But I will also incorporate this in the second submission for review .



Regards,
Jianyu Zhan