linux-kernel - Re: dcache: remove trylock loops (was Re: [BUG] lock_parent() breakage when used from shrink_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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20180312200540.GO30522@ZenIV.linux.org.uk>
Date:   Mon, 12 Mar 2018 20:05:41 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     NeilBrown <neilb@...e.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Eric Biederman <ebiederm@...ssion.com>
Subject: Re: dcache: remove trylock loops (was Re: [BUG] lock_parent()
 breakage when used from shrink_dentry_list())

On Mon, Mar 12, 2018 at 07:13:51PM +0000, Al Viro wrote:

> There's an unpleasant area around the ->s_root vs. NFS.  There's
> code that makes assumptions about ->s_root that are simply not true
> for NFS.  Is path_connected() correct wrt NFS multiple imports from
> the same server?  Ditto for mnt_already_visible() (that one might
> be mitigated at the moment, but probably won't last).  Eric, am
> I missing something subtle in there?

BTW, another thing spotted while trying to document the stuff:
Neil's "VFS: don't keep disconnected dentries on d_anon" has a subtle
side effect I hadn't spotted when applying.  Namely, for
DCACHE_DISCONNECTED non-directory IS_ROOT dentries we now have
d_unhashed() true.  That makes d_find_alias() logics around
discon_alias dead code:
                if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
                        if (IS_ROOT(alias) &&
                            (alias->d_flags & DCACHE_DISCONNECTED)) {
                                discon_alias = alias;
                        } else {
                                __dget_dlock(alias);
                                spin_unlock(&alias->d_lock);
                                return alias;
                        }
                }

Directory case is a red herring - we'll end up picking one and only
alias, whether it's IS_ROOT/disconnected or not.  For non-directories,
though, IS_ROOT and disconnected implies d_unhashed() now, so we might
as well turn that into
	if directory
		return d_find_any_alias
	else
		return the first hashed alias

Does any of the d_find_alias() callers want those dentries?  That is,
non-directories from d_obtain_alias() still not attached to a parent;
note that exportfs_decode_fh() is *not* one of such places - we don't
use d_find_alias() there at all.  If there's no such caller, we can
bloody well just drop the discon_alias logics and be done with that;
if there is, that commit has introduced a bug.  I might have missed
a part of threads related to that patch, so my apologies if it had
been discussed.

Neil, what's the situation there?  A lot of those callers clearly treat
the "only disconnected IS_ROOT alias exist" same as "no aliases at all";
it looks like the change might have been the right thing, but it sure
as hell shouldn't be an undocumented side effect...