DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C64AC7E42C
Subject: Re: [PATCH RFC 2/3] pipe: protect pipe_max_size access with a mutex
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Michael Kerrisk <mtk.manpages@gmail.com>,
        Randy Dunlap <rdunlap@infradead.org>
References: <1504622676-2992-1-git-send-email-joe.lawrence@redhat.com>
 <1504622676-2992-3-git-send-email-joe.lawrence@redhat.com>
 <alpine.LRH.2.02.1709141905370.17487@file01.intranet.prod.int.rdu2.redhat.com>
 <29f65845-40e5-839d-0bc7-79a5c1fa6f4b@redhat.com>
 <alpine.LRH.2.02.1709190339020.15590@file01.intranet.prod.int.rdu2.redhat.com>
From: Joe Lawrence <joe.lawrence@redhat.com>
Organization: Red Hat
Message-ID: <4121a697-d2fb-ec65-eea3-3c01592c8e08@redhat.com>
Date: Tue, 19 Sep 2017 17:32:14 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <alpine.LRH.2.02.1709190339020.15590@file01.intranet.prod.int.rdu2.redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2201
Lines: 58

On 09/19/2017 03:53 AM, Mikulas Patocka wrote:
> On Fri, 15 Sep 2017, Joe Lawrence wrote:
> [ ... snip ... ]
>> Hi Mikulas,
>>
>> I'm not strong when it comes to memory barriers, but one of the
>> side-effects of using the mutex is that pipe_set_size() and
>> alloc_pipe_info() should have a consistent view of pipe_max_size.
>>
>> If I remove the mutex (and assume that I implement a custom
>> do_proc_dointvec "conv" callback), is it safe for these routines to
>> directly use pipe_max_size as they had done before?
>>
>> If not, is it safe to alias through a temporary stack variable (ie,
>> could the compiler re-read pipe_max_size multiple times in the function)?
>>
>> Would READ_ONCE() help in any way?
> 
> Theoretically re-reading the variable is possible and you should use 
> ACCESS_ONCE or READ_ONCE+WRITE_ONCE on that variable.
> 
> In practice, ACCESS_ONCE/READ_ONCE/WRITE_ONCE is missing at a lot of 
> kernel variables that could be modified asynchronously and no one is 
> complaining about it and no one is making any systematic effort to fix it.
> 
> That re-reading happens (I have some test code that makes the gcc 
> optimizer re-read a variable), but it happens very rarely.

This would be interesting to look at if you are willing to share (can
send offlist).

> Another theoretical problem is that when reading or writing a variable 
> without ACCESS_ONCE, the compiler could read and write the variable using 
> multiple smaller memory accesses. But in practice, it happens only on some 
> non-common architectures.

Smaller access than word size?

>> The mutex covered up some confusion on my part here.
>>
>> OTOH, since pipe_max_size is read-only for pipe_set_size() and
>> alloc_pipe_info() and only updated occasionally by pipe_proc_fn(), would
>> rw_semaphore or RCU be a fit here?
> 
> RW semaphore causes cache-line ping-pong between CPUs, it slows down the 
> kernel just like a normal spinlock or mutex.

Ah right.

> RCU would be useless here (i.e. you don't want to allocate memory and 
> atomically assign it with rcu_assign_pointer).

And good point here.

Thanks for the explanations, they confirm and expand what I as already
thinking in this space.

--- Joe