linux-kernel - Re: [PATCH 1/5] locking: Introduce range reader/writer lock

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eef28924b290b3d9e737a4527002fc8b@suse.de>
Date:   Thu, 30 Mar 2017 10:13:12 -0700
From:   Davidlohr Bueso <dbueso@...e.de>
To:     Laurent Dufour <ldufour@...ux.vnet.ibm.com>
Cc:     Davidlohr Bueso <dave@...olabs.net>, mingo@...nel.org,
        peterz@...radead.org, akpm@...ux-foundation.org, jack@...e.cz,
        kirill.shutemov@...ux.intel.com, mhocko@...e.com,
        mgorman@...hsingularity.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] locking: Introduce range reader/writer lock

On 2017-03-30 07:56, Laurent Dufour wrote:
> On 07/03/2017 06:03, Davidlohr Bueso wrote:
>> +static inline int wait_for_ranges(struct range_rwlock_tree *tree,
>> +				  struct range_rwlock *lock, long state)
>> +{
>> +	int ret = 0;
>> +
>> +	while (true) {
>> +		set_current_state(state);
>> +
>> +		if (unlikely(signal_pending_state(state, current))) {
>> +			unsigned long flags;
>> +
>> +			ret = -EINTR;
>> +			/*
>> +			 * We're not taking the lock after all, cleanup
>> +			 * after ourselves.
>> +			 */
>> +			spin_lock_irqsave(&tree->lock, flags);
>> +			lock->reader = false;
>> +			__range_tree_remove(tree, lock);
>> +			spin_unlock_irqrestore(&tree->lock, flags);
>> +			break;
>> +		}
>> +
>> +		/* do we need to go to sleep? */
>> +		if (!lock->blocking_ranges)
>> +			break;
> 
> Hi Davidlohr,
> 
> While building a kernel on top of a patched kernel using full range 
> lock
> in the place of mmap_sem, I found that fork() sometimes failed 
> returning
> ENOMEM.
> It happens that if fork() get called at the time signal is sent to the
> calling process, the call to range_write_lock_interruptible() is 
> failing
> even if there is no contention on the lock. This is because we check 
> for
> the signal pending before checking for the lock contention in
> wait_for_ranges().
> 
> The loop here should rather be :
> 
> 	while (true) {
> 		/* do we need to go to sleep? */
> 		if (!lock->blocking_ranges)
> 			break;
> 

Thanks, this makes sense, and is actually the standard way of waiting in
most locks.

> 		if (unlikely(signal_pending_state(state, current))) {
> 			unsigned long flags;
> 
> 			ret = -EINTR;
> 			/*
> 			 * We're not taking the lock after all, cleanup
> 			 * after ourselves.
> 			 */
> 			spin_lock_irqsave(&tree->lock, flags);
> 			lock->reader = false;
> 			__range_tree_remove(tree, lock);
> 			spin_unlock_irqrestore(&tree->lock, flags);
> 			break;
> 		}
> 
> 		set_current_state(state);
> 
> 		schedule();
> 	}
> 
> I also moved the call to set_current_state() before calling schedule(),
> not sure this has to be done this way, but my system seems to work fine
> like this.

No, we do not hold any locks. Please keep set_current_state() the very 
first
thing we do in the loop. You can check Documentation/memory-barriers.txt
for details :-)

Thanks,
Davidlohr