Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] ipc/sem.c: use READ_ONCE()/WRITE_ONCE() for
 use_global_lock
To:     paulmck@kernel.org
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Andrew Morton <akpm@linux-foundation.org>, 1vier1@web.de
References: <20210514175319.12195-1-manfred@colorfullife.com>
 <20210514194407.GN975577@paulmck-ThinkPad-P17-Gen-1>
From:   Manfred Spraul <manfred@colorfullife.com>
Message-ID: <c3a1f1a3-8fc1-29b5-92e0-bf45e1cc438f@colorfullife.com>
Date:   Fri, 14 May 2021 22:25:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20210514194407.GN975577@paulmck-ThinkPad-P17-Gen-1>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Hi Paul,

On 5/14/21 9:44 PM, Paul E. McKenney wrote:
> On Fri, May 14, 2021 at 07:53:19PM +0200, Manfred Spraul wrote:
>> The patch solves two weaknesses in ipc/sem.c:
>>
>> 1) The initial read of use_global_lock in sem_lock() is an
>> intentional race. KCSAN detects these accesses and prints
>> a warning.
>>
>> 2) The code assumes that plain C read/writes are not
>> mangled by the CPU or the compiler.
>>
>> To solve both issues, use READ_ONCE()/WRITE_ONCE().
>> Plain C reads are used in code that owns sma->sem_perm.lock.
>>
>> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
>
> One follow-up question: If I am reading the code correctly, there is
> a call to complexmode_enter() from sysvipc_sem_proc_show() that does
> not hold the global lock.  Does this mean that the first check of
> ->use_global_lock in complexmode_enter() should be marked?

Now you made me nervous, usually I do not test the proc interface.
According to the documentation in sysvipc_sem_proc_show(), 
sysvipc_find_ipc() acquires the global lock.

>         /*
>          * The proc interface isn't aware of sem_lock(), it calls
>          * ipc_lock_object() directly (in sysvipc_find_ipc).
>          * In order to stay compatible with sem_lock(), we must
>          * enter / leave complex_mode.
>          */
I have just tested it again: Yes, this is still true.

Perhaps, as future improvement: The rest of ipc/sem.c speaks about 
"sem_perm.lock", and here we suddenly use a function name instead of the 
structure member name.

 > "it calls ipc_lock_object() (i.e.: spin_lock(&sma->sem_perm.lock)).

> 							Thanx, Paul
>
>> ---
>>   ipc/sem.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/ipc/sem.c b/ipc/sem.c
>> index bf534c74293e..a0ad3a3edde2 100644
>> --- a/ipc/sem.c
>> +++ b/ipc/sem.c
>> @@ -217,6 +217,8 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it);
>>    * this smp_load_acquire(), this is guaranteed because the smp_load_acquire()
>>    * is inside a spin_lock() and after a write from 0 to non-zero a
>>    * spin_lock()+spin_unlock() is done.
>> + * To prevent the compiler/cpu temporarily writing 0 to use_global_lock,
>> + * READ_ONCE()/WRITE_ONCE() is used.
>>    *
>>    * 2) queue.status: (SEM_BARRIER_2)
>>    * Initialization is done while holding sem_lock(), so no further barrier is
>> @@ -342,10 +344,10 @@ static void complexmode_enter(struct sem_array *sma)
>>   		 * Nothing to do, just reset the
>>   		 * counter until we return to simple mode.
>>   		 */
>> -		sma->use_global_lock = USE_GLOBAL_LOCK_HYSTERESIS;
>> +		WRITE_ONCE(sma->use_global_lock, USE_GLOBAL_LOCK_HYSTERESIS);
>>   		return;
>>   	}
>> -	sma->use_global_lock = USE_GLOBAL_LOCK_HYSTERESIS;
>> +	WRITE_ONCE(sma->use_global_lock, USE_GLOBAL_LOCK_HYSTERESIS);
>>   
>>   	for (i = 0; i < sma->sem_nsems; i++) {
>>   		sem = &sma->sems[i];
>> @@ -371,7 +373,8 @@ static void complexmode_tryleave(struct sem_array *sma)
>>   		/* See SEM_BARRIER_1 for purpose/pairing */
>>   		smp_store_release(&sma->use_global_lock, 0);
>>   	} else {
>> -		sma->use_global_lock--;
>> +		WRITE_ONCE(sma->use_global_lock,
>> +				sma->use_global_lock-1);
>>   	}
>>   }
>>   
>> @@ -412,7 +415,7 @@ static inline int sem_lock(struct sem_array *sma, struct sembuf *sops,
>>   	 * Initial check for use_global_lock. Just an optimization,
>>   	 * no locking, no memory barrier.
>>   	 */
>> -	if (!sma->use_global_lock) {
>> +	if (!READ_ONCE(sma->use_global_lock)) {
>>   		/*
>>   		 * It appears that no complex operation is around.
>>   		 * Acquire the per-semaphore lock.
>> -- 
>> 2.31.1
>>