linux-kernel - Re: fs: lockup on rename

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

Message-ID: <20141026191332.GS7996@ZenIV.linux.org.uk>
Date:	Sun, 26 Oct 2014 19:13:32 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Sasha Levin <sasha.levin@...cle.com>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Dave Jones <davej@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: fs: lockup on rename_mutex in fs/dcache.c:1035

On Sun, Oct 26, 2014 at 11:56:08AM -0700, Linus Torvalds wrote:
> On Sat, Oct 25, 2014 at 8:57 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> Your patch looks fine, and I don't think we can livelock - because we
> always set 'seq' to 1 if we retry, and that causes us to get the
> exclusive lock, so we'd better not then enter the retry loop again.
> Although I guess not only renames cause it - looks like we get to that
> mis-named "rename_retry" for a deletion too according to the comment.
> Although I'm not sure that comment is correct - I don't think we
> change d_parent for deletes, do we?

The comment is not correct.  dentry_kill() won't screw the pointer to
parent; it will, however, screw the pointer to next sibling.

It used to screw the pointer to parent (which is what the first part of
condition was about).  After Nick's series back in January 2011 that
has stopped being true.  However, dentry_kill() does
list_del(&dentry->d_u.d_child).  Which means that we can't continue
past that point if it has happened.  Trond has noticed the breakage
a bit later and added explicit check for ->d_flags, but the damage
was more extensive - Nick had missed the restarts-on-killed logics
hidden in the check for changed ->d_parent and assumed that it's
all about renames, meaning that once rename_lock has been taken exclusive
we won't have restarts at all.  With restart-on-killed restored that
wasn't true anymore, invalidating the assumption that we only get to
rename_retry without rename_lock held exclusive.  With deadlocks happening
if we ever get there on such pass.

We used to have several copies of that logics, before it all got consolidated
into d_walk(), which probably contributed to the mess...

BTW, the first part of condition (->d_parent hasn't changed since before the
"unlock child, lock parent" sequence) is really pointless these days - check
for rename_lock will fail if ->d_parent has changed...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/