linux-kernel - Re: [RFC][PATCHSET v3] non-recursive pathname resolution & RCU symlinks

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

Date:	Sat, 16 May 2015 12:36:04 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Dave Chinner <david@...morbit.com>,
	Andreas Dilger <adilger@...ger.ca>, NeilBrown <neilb@...e.de>,
	Christoph Hellwig <hch@...radead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFC][PATCHSET v3] non-recursive pathname resolution & RCU symlinks

On Fri, May 15, 2015 at 9:31 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
=>
> Point, but...  A lot of our problems comes from the fact that ->i_mutex
> doubles as protection against the addition to the list of children, on
> top of protection of directory itself.

Yeah, ok, we'd need to change that too. Maybe just make it use d_lock..

But yes, I like your alternative:

> What if we do the following:
> have the normal case of __lookup_hash() (and other callers of lookup_real())
>         * allocate dentry, marked "in-lookup"
>         * do dcache lookup, likely to come up empty, _without_ touching
> potential matches' d_lock, i.e. based on __d_lookup_rcu() (under
> rcu_read_lock(), with rename_lock loop around it).  Hold parent's ->d_lock
> while walking the chain, grab refcount in the unlikely case the match had
> been found.  If nothing's found *and* rename_lock hadn't been touched, insert
> the new dentry into hash and list of children before dropping ->d_lock.
>         * call ->lookup() (still under ->i_mutex, shared)
>         * clear "in-lookup" bit on _original_ dentry (we might very well
> have returned a different one)
>         * kick the wait queue of parent's ->i_mutex

I agree, that should work too, and might be somewhat advantageous. And
we do have that extra dentry, since we pass it down (for the name) to
lookup anyway. We'd just hash it and have that magical state.

Anyway, just grepping for "i_mutex" made me almost cry.

So phase 1 should probably be to not even touch i_mutex, but just add
a new abstraction layer to get rid of the direct lock accesses. That
will make things easier down the line.

The attached patch is huge, but it's all automated, and shouldn't
change any semantics at all - except to make it much easier to change
the locking details later. What do you think?

There are still a lot of "i_mutex" references in comments (several of
them clearly just mindless search-and-replace from when it used to be
a semaphore, when we _didn't_ do this cleanup: look for "down"
mentions ;) and there's a few scattered actual uses for initialization
and for two cases of 'mutex_lock_killable()' that I didn't bother to
make a wrapper for etc. But this should make it much easier to change
things eventually if we want to (ie we could turn it all into a rwsem
with a rather small patch for testing)

                            Linus

Download attachment "patch.diff.gz" of type "application/x-gzip" (52406 bytes)