Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE

[Jason Low]

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.