linux-kernel - Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE

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: <20160523201502.GN3825@linux.vnet.ibm.com>
Date:	Mon, 23 May 2016 13:15:02 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Davidlohr Bueso <dave@...olabs.net>
Cc:	Jason Low <jason.low2@....com>,
	Peter Hurley <peter@...leysoftware.com>, jason.low2@...com,
	Waiman Long <waiman.long@....com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Dave Chinner <david@...morbit.com>,
	Scott J Norton <scott.norton@....com>,
	Douglas Hatch <doug.hatch@....com>
Subject: Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by
 WRITE_ONCE

On Mon, May 23, 2016 at 12:44:16PM -0700, Davidlohr Bueso wrote:
> On Mon, 23 May 2016, Jason Low wrote:
> 
> >On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote:
> >>On 05/18/2016 12:58 PM, Jason Low wrote:
> >>> It should be fine to use the standard READ_ONCE here, even if it's just
> >>> for documentation, as it's probably not going to cost anything in
> >>> practice. It would be better to avoid adding any special macros for this
> >>> which may just add more complexity.
> >>
> >>See, I don't understand this line of reasoning at all.
> >>
> >>I read this as "it's ok to be non-optimal here where were spinning CPU
> >>time but not ok to be non-optimal generally elsewhere where it's
> >>way less important like at init time".
> >
> >So I think there is a difference between using it during init time and
> >using it here where we're spinning. During init time, initializing the
> >owner field locklessly is normal. No other thread should be concurrently
> >be writing to the field, since the structure is just getting
> >initialized, so there are no surprises there.
> >
> >Our access of the owner field in this function is special in that we're
> >using a bit of "lockless magic" to read and write to a field that gets
> >concurrently accessed without any serialization. Since we're not taking
> >the wait_lock in a scenario where we'd normally would take a lock, it
> >would be good to have this documented.
> >
> >>And by the way, it's not just "here" but _everywhere_.
> >>What about reading ->on_cpu locklessly?
> >
> >Sure, we could also use READ_ONCE when reading ->on_cpu  :)
> 
> Locking wise we should be covered with ->on_cpu as we're always under rcu_read_lock
> (barrier, preempt_disable). But I'm not sure if this rule applies throughout the
> scheduler, however, like it does in, say thread_group_cputime(). cpu_clock_sample()
> (from posix timers) seems to mix and match being done under rcu. So ultimately I
> think you're right.

But rcu_read_lock() does not exclude updates, which is one reason why
pointer reads use rcu_dereference() rather than normal assignments.

So I do not believe that rcu_read_lock() is helping you in this case.

That said, it is a bit hard to imagine the compiler tearing a load from
an int...  But compilers have uncovered weaknesses in my imagination
more than once in the past.

							Thanx, Paul