linux-kernel - Re: kernel panic: corrupted stack end in dput

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: <20190702132147.GG17978@ZenIV.linux.org.uk>
Date:   Tue, 2 Jul 2019 14:21:47 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Hillf Danton <hdanton@...a.com>
Cc:     syzbot <syzbot+d88a977731a9888db7ba@...kaller.appspotmail.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: kernel panic: corrupted stack end in dput

On Tue, Jul 02, 2019 at 05:21:26PM +0800, Hillf Danton wrote:

> Hello,

> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -673,14 +673,11 @@ static struct dentry *dentry_kill(struct dentry *dentry)
> 	if (!IS_ROOT(dentry)) {
> 		parent = dentry->d_parent;
> 		if (unlikely(!spin_trylock(&parent->d_lock))) {
> -			parent = __lock_parent(dentry);
> -			if (likely(inode || !dentry->d_inode))
> -				goto got_locks;
> -			/* negative that became positive */
> -			if (parent)
> -				spin_unlock(&parent->d_lock);
> -			inode = dentry->d_inode;
> -			goto slow_positive;
> +			/*
> +			 * fine if peer is busy either populating or
> +			 * cleaning up parent
> +			 */
> +			parent = NULL;
> 		}
> 	}
> 	__dentry_kill(dentry);

This is very much *NOT* fine.
	1) trylock can fail from any number of reasons, starting
with "somebody is going through the hash chain doing a lookup on
something completely unrelated"
	2) whoever had been holding the lock and whatever they'd
been doing might be over right after we get the return value from
spin_trylock().
	3) even had that been really somebody adding children in
the same parent *AND* even if they really kept doing that, rather
than unlocking and buggering off, would you care to explain why
dentry_unlist() called by __dentry_kill() and removing the victim
from the list of children would be safe to do in parallel with that?

NAK, in case it's not obvious from the above.