On Tue, 2006-09-19 at 10:24 +0200, Miklos Szeredi wrote: > > A: cd /mnt/a/b > > B: mv /mnt/a/b /mnt > > B: mv /mnt/a /mnt/b > > A: cd a > > I think with this it's possible (though not easy) to deadlock NFS: Actually, it is dead(sic!) easy. See my testcase in http://bugzilla.kernel.org/show_bug.cgi?id=7178 Sigh... I'm thinking something along the lines of the following patch on top of David's d_materialise_dentry(), which is already in the 2.6.18-mm kernels. Still untested (I'm working on that), but it attempts to do what you were proposing: 1) If instantiating a directory, check for aliases 2) If an alias is found in the dcache, attempt to rename the existing dentry instead of instantiating the alias. 3) Return ELOOP if the alias turns out to be a parent. Cheers, Trond
--- Begin Message ---
- Subject: No Subject
- From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
- Date:
- Vfs: Make d_materialise_unique() enforce directory uniqueness
If the caller tries to instantiate a directory using an inode that already has a dentry alias, then we attempt to rename the existing dentry instead of instantiating a new one. Fail with an ELOOP error if the rename would affect one of our parent directories. This behaviour is needed in order to avoid issues such as http://bugzilla.kernel.org/show_bug.cgi?id=7178 Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx> --- fs/dcache.c | 79 +++++++++++++++++++++++++++++++++++++--------------------- fs/nfs/dir.c | 7 ++++- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 17b392a..2bbf422 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1337,23 +1337,21 @@ static void switch_names(struct dentry * * deleted it. */ -/** - * d_move - move a dentry +/* + * d_move_locked - move a dentry * @dentry: entry to move * @target: new dentry * * Update the dcache to reflect the move of a file name. Negative * dcache entries should not be moved in this way. */ - -void d_move(struct dentry * dentry, struct dentry * target) +static void d_move_locked(struct dentry * dentry, struct dentry * target) { struct hlist_head *list; if (!dentry->d_inode) printk(KERN_WARNING "VFS: moving negative dcache entry\n"); - spin_lock(&dcache_lock); write_seqlock(&rename_lock); /* * XXXX: do we really need to take target->d_lock? @@ -1404,10 +1402,39 @@ already_unhashed: fsnotify_d_move(dentry); spin_unlock(&dentry->d_lock); write_sequnlock(&rename_lock); +} + +/** + * d_move - move a dentry + * @dentry: entry to move + * @target: new dentry + * + * Update the dcache to reflect the move of a file name. Negative + * dcache entries should not be moved in this way. + */ + +void d_move(struct dentry * dentry, struct dentry * target) +{ + spin_lock(&dcache_lock); + d_move_locked(dentry, target); spin_unlock(&dcache_lock); } /* + * Reject any lookup loops due to malicious remote servers + */ +static int d_detect_loops(struct dentry *dentry, struct inode *inode) +{ + struct dentry *p; + + for (p = dentry; p->d_parent != p; p = p->d_parent) { + if (p->d_parent->d_inode == inode) + return -ELOOP; + } + return 0; +} + +/* * Prepare an anonymous dentry for life in the superblock's dentry tree as a * named dentry in place of the dentry to be replaced. */ @@ -1461,26 +1488,22 @@ struct dentry *d_materialise_unique(stru goto found_lock; } - /* See if a disconnected directory already exists as an anonymous root - * that we should splice into the tree instead */ - if (S_ISDIR(inode->i_mode) && (alias = __d_find_alias(inode, 1))) { - spin_lock(&alias->d_lock); - - /* Is this a mountpoint that we could splice into our tree? */ - if (IS_ROOT(alias)) - goto connect_mountpoint; - - if (alias->d_name.len == dentry->d_name.len && - alias->d_parent == dentry->d_parent && - memcmp(alias->d_name.name, - dentry->d_name.name, - dentry->d_name.len) == 0) - goto replace_with_alias; - - spin_unlock(&alias->d_lock); - - /* Doh! Seem to be aliasing directories for some reason... */ - dput(alias); + if (S_ISDIR(inode->i_mode)) { + actual = ERR_PTR(d_detect_loops(dentry, inode)); + if (IS_ERR(actual)) + goto out_unlock; + /* Does an aliased dentry already exist? */ + alias = __d_find_alias(inode, 0); + if (alias) { + actual = alias; + /* Is this an anonymous mountpoint that we could splice + * into our tree? */ + if (IS_ROOT(alias)) + goto connect_mountpoint; + /* Nope, but we must(!) avoid directory aliasing */ + d_move_locked(alias, dentry); + goto out_unlock; + } } /* Add a unique reference */ @@ -1495,6 +1518,7 @@ found_lock: found: _d_rehash(actual); spin_unlock(&actual->d_lock); +out_unlock: spin_unlock(&dcache_lock); if (actual == dentry) { @@ -1507,12 +1531,9 @@ found: /* Convert the anonymous/root alias into an ordinary dentry */ connect_mountpoint: + spin_lock(&alias->d_lock); __d_materialise_dentry(dentry, alias); - - /* Replace the candidate dentry with the alias in the tree */ -replace_with_alias: __d_drop(alias); - actual = alias; goto found; shouldnt_be_hashed: diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 3419c2d..aca90cb 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -933,8 +933,11 @@ static struct dentry *nfs_lookup(struct no_entry: res = d_materialise_unique(dentry, inode); - if (res != NULL) + if (res != NULL) { + if (IS_ERR(res)) + goto out_unlock; dentry = res; + } nfs_renew_times(dentry); nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); out_unlock: @@ -1130,6 +1133,8 @@ static struct dentry *nfs_readdir_lookup alias = d_materialise_unique(dentry, inode); if (alias != NULL) { dput(dentry); + if (IS_ERR(alias)) + return NULL; dentry = alias; }
--- End Message ---
