linux-kernel - Stupid VFS name lookup interface..

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

Message-ID: <CA+55aFyKg=m=USnRmFQNaRB_9MWNpzdLwxr-v7X0ONaG72+nsg@mail.gmail.com>
Date:	Tue, 21 May 2013 15:22:44 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Stupid VFS name lookup interface..

Ok, Al, please tell me why I'm wrong, but I was looking at the hot
code in fs/dcache.c again (__d_lookup_rcu() remains the hottest
function under pathname lookup heavy operations) and that "inode"
argument was mis-commented (it used to be an in-out argument long long
ago) and it just kept bugging me.

And it looks totally pointless anyway. Nobody sane actually wants it.

Yeah, several filesystems use "pinode->i_sb" to look up their
superblock, but they can use "dentry->d_sb" for that instead. You did
most of that long ago, I think.

The *one* insane exception is ncpfs, which actually wants to look at
the parent (ie directory) inode data in order to decide if it should
use a case sensitive hash or not. However, even in that case, I'd
argue that we could just optimistically do a
ACCESS_ONCE(dentry->d_inode) and do the compare using the information
we got from that.

Because we don't care if the dentry->d_inode is unstable: if we got
some stale inode, we would hit the dentry_rcuwalk_barrier() case for
that parent when we later check the sequence numbers. So then we'd
throw away the comparison result anyway.  We check both the dentry and
the parent sequence count in lookup_fast(), verifying that they've
been stable over the sequence.

So as far as I can tell, the only thing we should worry about might be
a NULL pointer due to a concurrent rmdir(), but the identity of the
inode itself we really don't care too much about. Take one or the
other, and don't crash on NULL.

There's a similat case going on in proc_sys_compare(). Same logic applies.

Hmm?

Getting rid of those annoying separate dentry inode pointers makes
__d_lookup_rcu() compile into noticeably better code on x86-64,
because there's no stack frame needed any more. And all the
filesystems end up cleaner too, and the crazy cases go where they
belong.

Untested patch attached. It compiles cleanly, looks sane, and most of
it is just making the function prototypes look much nicer. I think it
works.

              Linus

Download attachment "patch.diff" of type "application/octet-stream" (34655 bytes)