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]

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

On Wed, 2016-05-18 at 14:29 -0400, Waiman Long wrote:
> On 05/18/2016 01:21 PM, Jason Low wrote:
> > On Wed, 2016-05-18 at 07:04 -0700, Davidlohr Bueso wrote:
> >> On Tue, 17 May 2016, Waiman Long wrote:
> >>
> >>> Without using WRITE_ONCE(), the compiler can potentially break a
> >>> write into multiple smaller ones (store tearing). So a read from the
> >>> same data by another task concurrently may return a partial result.
> >>> This can result in a kernel crash if the data is a memory address
> >>> that is being dereferenced.
> >>>
> >>> This patch changes all write to rwsem->owner to use WRITE_ONCE()
> >>> to make sure that store tearing will not happen. READ_ONCE() may
> >>> not be needed for rwsem->owner as long as the value is only used for
> >>> comparison and not dereferencing.
> > It might be okay to leave out READ_ONCE() for reading rwsem->owner, but
> > couldn't we include it to at least document that we're performing a
> > "special" lockless read?
> >
> 
> Using READ_ONCE() does have a bit of cost as it limits compiler 
> optimization. If we changes all access to rwsem->owner to READ_ONCE() 
> and WRITE_ONCE(), we may as well change its type to volatile and be done 
> with.

Right, although there are still places like the init function where
WRITE_ONCE isn't necessary.

> I am not against doing that, but it feels a bit over-reach for me. 
> On the other hand, we may define a do-nothing macro that designates the 
> owner as a special variable for documentation purpose, but don't need 
> protection at that particular call site.

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.