From: Trond Myklebust <[email protected]>
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 <[email protected]>
---
fs/dcache.c | 102 +++++++++++++++++++++++++++++++++++++---------------------
fs/nfs/dir.c | 7 +++-
2 files changed, 71 insertions(+), 38 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 2bac4ba..f700afd 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1469,23 +1469,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?
@@ -1536,10 +1534,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.
*/
@@ -1581,7 +1608,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));
@@ -1593,26 +1620,36 @@ 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 (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 */
@@ -1627,8 +1664,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;
@@ -1637,16 +1675,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 c86a1ea..2cfa414 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -935,8 +935,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:
@@ -1132,6 +1135,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;
}
From: Trond Myklebust <[email protected]>
If someone has renamed a directory on the server, triggering the d_move
code in d_materialise_unique(), then we need to invalidate the
cached directory information in the source parent directory.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 2cfa414..fa71936 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -936,8 +936,14 @@ static struct dentry *nfs_lookup(struct
no_entry:
res = d_materialise_unique(dentry, inode);
if (res != NULL) {
+ struct dentry *parent;
if (IS_ERR(res))
goto out_unlock;
+ /* Was a directory renamed! */
+ parent = dget_parent(res);
+ if (!IS_ROOT(parent))
+ nfs_mark_for_revalidate(parent->d_inode);
+ dput(parent);
dentry = res;
}
nfs_renew_times(dentry);
> From: Trond Myklebust <[email protected]>
>
> 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
>
This looks like a stale patch. You posted one on -fsdevel that did
i_mutex locking as well.
Miklos
On Thu, 2006-10-19 at 22:51 +0200, Miklos Szeredi wrote:
> > From: Trond Myklebust <[email protected]>
> >
> > 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
> >
>
> This looks like a stale patch. You posted one on -fsdevel that did
> i_mutex locking as well.
Argh! Thanks for spotting that Miklos... I had forgotten to update my
'fixes' stgit branch with the newer version. Please discard the previous
version of this patch, Linus. I'll resend...
Cheers,
Trond
From: Trond Myklebust <[email protected]>
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 <[email protected]>
---
fs/dcache.c | 137 ++++++++++++++++++++++++++++++++++++++++++----------------
fs/nfs/dir.c | 7 +++
2 files changed, 106 insertions(+), 38 deletions(-)
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 *
* 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(struc
*/
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(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;
+
+ /* 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();
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index c86a1ea..2cfa414 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -935,8 +935,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:
@@ -1132,6 +1135,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;
}
From: Trond Myklebust <[email protected]>
If someone has renamed a directory on the server, triggering the d_move
code in d_materialise_unique(), then we need to invalidate the
cached directory information in the source parent directory.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 2cfa414..fa71936 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -936,8 +936,14 @@ static struct dentry *nfs_lookup(struct
no_entry:
res = d_materialise_unique(dentry, inode);
if (res != NULL) {
+ struct dentry *parent;
if (IS_ERR(res))
goto out_unlock;
+ /* Was a directory renamed! */
+ parent = dget_parent(res);
+ if (!IS_ROOT(parent))
+ nfs_mark_for_revalidate(parent->d_inode);
+ dput(parent);
dentry = res;
}
nfs_renew_times(dentry);
On Thu, 19 Oct 2006 17:03:58 -0400
Trond Myklebust <[email protected]> wrote:
> +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;
> +}
The locking in there is, of course, gruesome. There is no way in which it
can be made reliable?
On Thu, 2006-10-19 at 21:40 -0700, Andrew Morton wrote:
> On Thu, 19 Oct 2006 17:03:58 -0400
> Trond Myklebust <[email protected]> wrote:
>
> > +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;
> > +}
>
> The locking in there is, of course, gruesome. There is no way in which it
> can be made reliable?
The generic lookup() code will grab the dir->i_mutex for the parent
directory before we get anywhere near __d_alias(). That pretty much
limits us to using mutex_trylock() since otherwise we break the nesting
rules for lock_rename(), and I can't see that we can release the
problematic dir->i_mutex without causing worse races.
Cheers,
Trond