On Fri, Feb 17, 2012 at 6:30 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> On Thu, Feb 16, 2012 at 02:06:03PM +1100, NeilBrown wrote:
>> I was going ask how you managed to get an 'unhashed' dentry which was not
>> DISCONNECTED, and belonged to a directory that could be the subject of
>> d_splice_alias (that implies it has a name).
>>
>> The bug sounds like a race between lookup and rmdir, which should be
>> prevented by i_mutex.
>>
>> I think that using __d_find_any_alias would just be papering over the
>> problem, and would trigger a BUG_ON when it returned a non-DISCONNECTED alias.
>
> Looking through the latest upstream code, I can't come up with another
> obvious reproducer.
>
> But I also can't see the code making any particular effort to ensure
> that dentries are removed from inode's alias lists at the same time
> they're unhashed.
>
> E.g., trace up through the callers of d_drop/__d_drop and try to
> convince yourselves that they all end up removing the dentry from the
> alias list.
>
> Can you see any reason why the following would actually create a
> problem?
>
> --b.
>
> commit fcfef6b7319c5d19ea5064317528ff994343b011
> Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> Date: Mon Feb 13 13:38:33 2012 -0500
>
> exports: stop d_splice_alias creating directory aliases
>
> A directory should never have more than one dentry pointing to it.
>
> But d_splice_alias() does so in the case of a directory with an
> already-existing non-DISCONNECTED dentry.
>
> Prior to the removal of dentry_unhash() from vfs_rmdir(), around v3.0,
> this could cause an nfsd deadlock like this:
>
> - Somebody attempts to remove a non-empty directory.
> - The dentry_unhash() in vfs_rmdir() unhashes the dentry
> pointing to the non-empty directory.
> - ->rmdir() then fails with -ENOTEMPTY
> - Before the vfs_rmdir() caller reaches dput(), an nfsd process
> in rename looks up the directory by filehandle; at the end of
> that lookup, this dentry is found by d_alloc_anon(), and a
> reference is taken on it, preventing dput() from removing it.
> - A regular lookup of the directory calls d_splice_alias(),
> finds only an unhashed (not a DISCONNECTED) dentry, and
> insteads adds a new one, so the directory now has two
> dentries.
> - The nfsd process in rename, which was previously looking up
> the source directory of the rename, now looks up the target
> directory (which is the same), and gets the dentry newly
> created by the previous lookup.
> - The rename, seeing two different dentries, assumes this is a
> cross-directory rename and attempts to take the i_mutex on the
> directory twice.
>
> I don't see as obvious a reproducer now, but I also don't see the
> existing code taking care to remove dentries from the alias list
> whenever they're unhashed.
>
> It therefore seems safest to allow d_splice_alias to use any dentry it
> finds.
>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index f68e193..1fd2256 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1602,9 +1602,8 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
>
> if (inode && S_ISDIR(inode->i_mode)) {
> spin_lock(&inode->i_lock);
> - new = __d_find_alias(inode, 1);
by replacing this, the want_discon argument is no longer in use. care
to remove it as well?
--
Thanks,
Tao
> + new = __d_find_any_alias(inode);
> if (new) {
> - BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
> spin_unlock(&inode->i_lock);
> security_d_instantiate(new, inode);
> d_move(new, dentry);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
