linux-kernel - Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d

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] [day] [month] [year] [list]

Message-ID: <87bmgh4e1t.fsf@linutronix.de>
Date:   Thu, 22 Feb 2018 09:35:42 +0100
From:   John Ogness <john.ogness@...utronix.de>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Christoph Hellwig <hch@....de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] fs/dcache: Avoid the try_lock loop in d_delete()

On 2018-02-22, Al Viro <viro@...IV.linux.org.uk> wrote:
>> @@ -2378,22 +2420,36 @@ void d_delete(struct dentry * dentry)
>>  	/*
>>  	 * Are we the only user?
>>  	 */
>> -again:
>>  	spin_lock(&dentry->d_lock);
>> +again:
>>  	inode = dentry->d_inode;
>>  	isdir = S_ISDIR(inode->i_mode);
>>  	if (dentry->d_lockref.count == 1) {
>> -		if (!spin_trylock(&inode->i_lock)) {
>> -			spin_unlock(&dentry->d_lock);
>> -			cpu_relax();
>> +		/*
>> +		 * Lock the inode. Might drop dentry->d_lock temporarily
>> +		 * which allows inode to change. Start over if that happens.
>> +		 */
>> +		if (!dentry_lock_inode(dentry))
>>  			goto again;
>
> IDGI.  First of all, why do we need to fetch ->d_inode (and calculate
> isdir) before that dentry_lock_inode() of yours? That's at least
> partially understandable in the current version, where we need inode
> in d_delete() scope, but here it looks bloody odd.

I tried to change the function as little as possible. You are right that
it now looks odd. I seem to have missed the forest for the trees.

> And if you move those fetches past the call of dentry_lock_inode(),
> you suddenly get the life much simpler:
>
> 	grab d_lock
> 	if d_count is greater than 1, drop it and bugger off
> 	while !dentry_lock_inode(dentry)
> 		;
> 	fetch inode
> 	recheck d_count, in the unlikely case when it's greater than 1,
> 			drop and bugger off
> 	clear CANT_MOUNT
> 	calculate isdir
> 	unlink_inode
> 	fsnotify shite
>
> I mean, do we really want to keep rechecking d_count on each loop
> iteration?  What does it buy us?  Sure, we want to recheck in the end
> for correctness sake, but...

I have been unable to produce a test case where dentry_lock_inode() can
fail. AFAICT it is not possible from userspace. Perhaps some filesystem
could trigger it. But if it would fail, getting the refcount to increase
in the dropped d_lock window is quite easy to reproduce. And in that
case we wouldn't need to keep trying to aquire the inode lock and could
just drop.
        
> It might make sense to move the loop inside dentry_lock_inode(), IMO.

Agreed. I will change dentry_lock_inode() so that it will only fail if
the refcount changes. If there are inode changes, it will loop
internally. That will change your suggestion to:

 	grab d_lock
 	if d_count is greater than 1
 	 	drop it and bugger off
 	if !dentry_lock_inode(dentry)
	 	drop it and bugger off
 	fetch inode
 	clear CANT_MOUNT
 	calculate isdir
 	unlink_inode
 	fsnotify shite

John Ogness