linux-kernel - Re: [PATCH v2 4/6] fs/dcache: Avoid the try_lock loops in dentry

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: <20180223022242.GV30522@ZenIV.linux.org.uk>
Date:   Fri, 23 Feb 2018 02:22:43 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     John Ogness <john.ogness@...utronix.de>
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 v2 4/6] fs/dcache: Avoid the try_lock loops in
 dentry_kill()

On Fri, Feb 23, 2018 at 12:50:23AM +0100, John Ogness wrote:
>  static struct dentry *dentry_kill(struct dentry *dentry)
>  	__releases(dentry->d_lock)
>  {
> -	struct inode *inode = dentry->d_inode;
> -	struct dentry *parent = NULL;
> +	int saved_count = dentry->d_lockref.count;

	Umm...  How can that be not 1?  After all, fast_dput() should
never return false without ->d_lock being held *and* ->d_count being
equal to 1.

> +	/*
> +	 * d_inode might have changed if d_lock was temporarily
> +	 * dropped. If it changed it is necessary to start over
> +	 * because a wrong inode (or no inode) lock is held.
> +	 */

If it might have changed, we are fucked.

> +out_ref_changed:
> +	/*
> +	 * The refcount was incremented while dentry->d_lock was dropped.
> +	 * Just decrement the refcount, unlock, and tell the caller to
> +	 * stop the directory walk.
> +	 */
> +	if (!WARN_ON(dentry->d_lockref.count < 1))
> +		dentry->d_lockref.count--;
> +
>  	spin_unlock(&dentry->d_lock);
> -	return dentry; /* try again with same dentry */
> +
> +	return NULL;

No.  This is completely wrong.  If somebody else has found the sucker
while we dropped the lock and even got around to playing with refcount,
they might have done more than that.

In particular, they might have *dropped* their reference, after e.g.
picking it as our inode's alias and rehashed the fucker.  Making
our decision not to retain it no longer valid.  And your code will
not notice that.