From 9eaef27b36a6b716384948da94b8fc5bfba7b712 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Sat, 21 Oct 2006 10:24:20 -0700 Subject: [PATCH] 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 Cc: Miklos Szeredi Cc: Maneesh Soni Cc: Dipankar Sarma Cc: Neil Brown Cc: Al Viro Cc: Christoph Hellwig Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/dcache.c | 137 ++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 100 insertions(+), 37 deletions(-) (limited to 'fs/dcache.c') diff --git a/fs/dcache.c b/fs/dcache.c index 2bac4ba..a1ff91e 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1469,23 +1469,21 @@ static void switch_names(struct dentry *dentry, struct dentry *target) * 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? @@ -1536,10 +1534,84 @@ 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); } /* + * Helper that returns 1 if p1 is a parent of p2, else 0 + */ +static int d_isparent(struct dentry *p1, struct dentry *p2) +{ + struct dentry *p; + + for (p = p2; p->d_parent != p; p = p->d_parent) { + if (p->d_parent == p1) + return 1; + } + return 0; +} + +/* + * This helper attempts to cope with remotely renamed directories + * + * It assumes that the caller is already holding + * dentry->d_parent->d_inode->i_mutex and the dcache_lock + * + * Note: If ever the locking in lock_rename() changes, then please + * remember to update this too... + * + * On return, dcache_lock will have been unlocked. + */ +static struct dentry *__d_unalias(struct dentry *dentry, struct dentry *alias) +{ + struct mutex *m1 = NULL, *m2 = NULL; + struct dentry *ret; + + /* If alias and dentry share a parent, then no extra locks required */ + if (alias->d_parent == dentry->d_parent) + goto out_unalias; + + /* Check for loops */ + ret = ERR_PTR(-ELOOP); + if (d_isparent(alias, dentry)) + goto out_err; + + /* See lock_rename() */ + ret = ERR_PTR(-EBUSY); + if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex)) + goto out_err; + m1 = &dentry->d_sb->s_vfs_rename_mutex; + if (!mutex_trylock(&alias->d_parent->d_inode->i_mutex)) + goto out_err; + m2 = &alias->d_parent->d_inode->i_mutex; +out_unalias: + d_move_locked(alias, dentry); + ret = alias; +out_err: + spin_unlock(&dcache_lock); + if (m2) + mutex_unlock(m2); + if (m1) + mutex_unlock(m1); + return ret; +} + +/* * Prepare an anonymous dentry for life in the superblock's dentry tree as a * named dentry in place of the dentry to be replaced. */ @@ -1581,7 +1653,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon) */ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode) { - struct dentry *alias, *actual; + struct dentry *actual; BUG_ON(!d_unhashed(dentry)); @@ -1593,26 +1665,27 @@ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode) 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; + + /* 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 */ + actual = __d_unalias(dentry, alias); + if (IS_ERR(actual)) + dput(alias); + goto out_nolock; + } } /* Add a unique reference */ @@ -1628,7 +1701,7 @@ found: _d_rehash(actual); spin_unlock(&actual->d_lock); spin_unlock(&dcache_lock); - +out_nolock: if (actual == dentry) { security_d_instantiate(dentry, inode); return NULL; @@ -1637,16 +1710,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(); -- cgit v1.1 From 6eac3f93f5e6b7256fb20b7608d62ec192da12be Mon Sep 17 00:00:00 2001 From: Vasily Averin Date: Sat, 28 Oct 2006 10:38:44 -0700 Subject: [PATCH] missing unused dentry in prune_dcache()? On the the following patch: http://linux.bkbits.net:8080/linux-2.6/gnupatch@449b144ecSF1rYskg3q-SeR2vf88zg # ChangeSet # 2006/06/22 15:05:57-07:00 neilb@suse.de # [PATCH] Fix dcache race during umount # If prune_dcache finds a dentry that it cannot free, it leaves it where it # is (at the tail of the list) and exits, on the assumption that some other # thread will be removing that dentry soon. However as far as I see this comment is not correct: when we cannot take s_umount rw_semaphore (for example because it was taken in do_remount) this dentry is already extracted from dentry_unused list and we do not add it into the list again. Therefore dentry will not be found by prune_dcache() and shrink_dcache_sb() and will leave in memory very long time until the partition will be unmounted. The patch adds this dentry into tail of the dentry_unused list. Signed-off-by: Vasily Averin Cc: Neil Brown Acked-by: David Howells Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/dcache.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'fs/dcache.c') diff --git a/fs/dcache.c b/fs/dcache.c index a1ff91e..a5b76b6 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -478,11 +478,12 @@ static void prune_dcache(int count, struct super_block *sb) up_read(s_umount); } spin_unlock(&dentry->d_lock); - /* Cannot remove the first dentry, and it isn't appropriate - * to move it to the head of the list, so give up, and try - * later + /* + * Insert dentry at the head of the list as inserting at the + * tail leads to a cycle. */ - break; + list_add(&dentry->d_lru, &dentry_unused); + dentry_stat.nr_unused++; } spin_unlock(&dcache_lock); } -- cgit v1.1 From f87135762de4328c6f17897e803e6909bc056feb Mon Sep 17 00:00:00 2001 From: David Howells Date: Sat, 28 Oct 2006 10:38:46 -0700 Subject: [PATCH] VFS: Fix an error in unused dentry counting With Vasily Averin Fix an error in unused dentry counting in shrink_dcache_for_umount_subtree() in which the count is modified without the dcache_lock held. Signed-off-by: David Howells Cc: Vasily Averin Cc: Neil Brown Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/dcache.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'fs/dcache.c') diff --git a/fs/dcache.c b/fs/dcache.c index a5b76b6..fd4a428 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -557,6 +557,7 @@ repeat: static void shrink_dcache_for_umount_subtree(struct dentry *dentry) { struct dentry *parent; + unsigned detached = 0; BUG_ON(!IS_ROOT(dentry)); @@ -621,7 +622,7 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry) atomic_dec(&parent->d_count); list_del(&dentry->d_u.d_child); - dentry_stat.nr_dentry--; /* For d_free, below */ + detached++; inode = dentry->d_inode; if (inode) { @@ -639,7 +640,7 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry) * otherwise we ascend to the parent and move to the * next sibling if there is one */ if (!parent) - return; + goto out; dentry = parent; @@ -648,6 +649,11 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry) dentry = list_entry(dentry->d_subdirs.next, struct dentry, d_u.d_child); } +out: + /* several dentries were freed, need to correct nr_dentry */ + spin_lock(&dcache_lock); + dentry_stat.nr_dentry -= detached; + spin_unlock(&dcache_lock); } /* -- cgit v1.1