On Thu, 2006-09-28 at 11:02 +0100, Al Viro wrote: > Simpler: if alias and our dentry share the parent, it's locked and > we can rename without s_vfs_rename_mutex. If they do not, walk the > tree under alias, unhash all non-directories and kill all directories. > > We keep local renames, we keep renames within directory and cross-directory > rename on server ends up with invalidation when client notices it. > > We don't _care_ if lookup() is not from rename. That's OK. You can't assume that you can always kill the subdirectories: they may be in use. Even if no actual processes are using them, I may have submounts. Forcing processes to wait until the administrator gets round to unmounting all subdirs of the old alias before they can access the new one is not going to be acceptable to most users. The attached patch is what I'm testing out right now. I've basically added Miklos' suggestion of trying to grab the s_vfs_rename_mutex in order to protect the cross-directory d_move() case, and then failing with an EBUSY if that is not possible. Cheers, Trond
--- Begin Message ---
- Subject: No Subject
- From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
- Date: Fri, 22 Sep 2006 15:32:14 -0400
- 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 | 106 ++++++++++++++++++++++++++++++++++++++-------------------- fs/nfs/dir.c | 7 +++- 2 files changed, 75 insertions(+), 38 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 17b392a..abe219f 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. */ @@ -1449,7 +1476,7 @@ static void __d_materialise_dentry(struc */ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode) { - struct dentry *alias, *actual; + struct dentry *actual; BUG_ON(!d_unhashed(dentry)); @@ -1461,26 +1488,40 @@ 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)) { + struct dentry *alias; + + 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)) { + spin_lock(&alias->d_lock); + __d_materialise_dentry(dentry, alias); + __d_drop(alias); + goto found; + } + /* Nope, but we must(!) avoid directory aliasing */ + if (alias->d_parent == dentry->d_parent) { + d_move_locked(alias, dentry); + goto out_unlock; + } + if (mutex_trylock(&inode->i_sb->s_vfs_rename_mutex)) { + d_move_locked(alias, dentry); + spin_unlock(&dcache_lock); + mutex_unlock(&inode->i_sb->s_vfs_rename_mutex); + goto out_nolock; + } + spin_unlock(&dcache_lock); + dput(alias); + actual = ERR_PTR(-EBUSY); + goto out_nolock; + } } /* Add a unique reference */ @@ -1495,8 +1536,9 @@ found_lock: found: _d_rehash(actual); spin_unlock(&actual->d_lock); +out_unlock: spin_unlock(&dcache_lock); - +out_nolock: if (actual == dentry) { security_d_instantiate(dentry, inode); return NULL; @@ -1505,16 +1547,6 @@ found: iput(inode); return actual; - /* Convert the anonymous/root alias into an ordinary dentry */ -connect_mountpoint: - __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: spin_unlock(&dcache_lock); BUG(); diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 7432f1a..0f22a26 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 ---
