linux-kernel - Re: [PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

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: <20140929155918.GG7996@ZenIV.linux.org.uk>
Date:	Mon, 29 Sep 2014 16:59:18 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Mikhail Efremov <sem@...linux.org>
Subject: Re: [PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

On Mon, Sep 29, 2014 at 08:15:51AM -0700, Linus Torvalds wrote:
> On Sun, Sep 28, 2014 at 11:05 AM, Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > Folks, care to review and test the following?
> 
> No testing, but having thought about this some more, I'm personally
> getting quite convinced that doing the RCU delaying of the external
> name freeing in the __d_free() path is entirely pointless.
> 
> So I think the *only* rcu_free() you need is for just the "free old
> name" case in copy_name().
> 
> In __d_free(), the name pointer has gone through the same grace period
> that the dentry pointer itself went through. If it's not safe to free
> the external name, then it damn well wouldn't be safe to free the
> dentry itself either.
> 
> IOW, I think your games in __d_free() are totally unnecessary.
> 
> Now you can tell me why I'm wrong.

Sure.  Three dentries.  move the first over the second, dput the second.
The name is shared, the second dentry unhashed and the last reference
dropped.  free_dentry(second) does call_rcu(__d_free, ...).

Now RCU lookup starts.  And on another CPU we move the first dentry over
the third one.  copy_name() is called, it decrements the refcount down
to 1 (__d_free() hasn't happened yet) and doesn't schedule any freeing.

RCU lookup sees first dentry, still with the old name (what used to be the
name of the second dentry).

... and __d_free() on the second dentry is called.  call_rcu() has happened
before RCU lookup has done its rcu_read_lock(), so it's not required to
wait.  We decrement the refcount on ext name, it reaches 0 and we kfree()
it.  Right under dentry_cmp().  Oops...

We really need to decrement refcount before RCU-delaying.  IOW, the decrement
should be in dentry_free(), not in __d_free()...
--
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/