* [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases
2012-03-13 20:55 [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries J. Bruce Fields
@ 2012-03-13 20:58 ` J. Bruce Fields
0 siblings, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2012-03-13 20:58 UTC (permalink / raw)
To: Al Viro
Cc: Peng Tao, NeilBrown, Al Viro, Nick Piggin, Nick Piggin, linux-nfs,
linux-fsdevel, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
A directory should never have more than one dentry pointing to it.
But d_splice_alias() will add one if it finds a directory with an
already-existing non-DISCONNECTED dentry.
I can't find an obvious reproducer, but I also can't see what prevents
d_splice_alias() from encountering such a case.
It therefore seems safest to allow d_splice_alias to use any dentry it
finds.
(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.
That reproducer no longer exists, but I don't think there was anything
fundamentally incorrect about the vfs_rmdir() behavior there, so I think
the real fault was here in d_splice_alias().)
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/dcache.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 16a53cc..30f4236 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1587,9 +1587,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);
+ 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);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* DCACHE_DISCONNECTED patches
@ 2012-05-09 21:18 J. Bruce Fields
[not found] ` <1336598286-28636-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2012-05-09 21:18 UTC (permalink / raw)
To: Al Viro
Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA
This is a resend of a couple DCACHE_DISCONNECTED patches that I think
could go into 3.5.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases
[not found] ` <1336598286-28636-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2012-05-09 21:18 ` J. Bruce Fields
2012-05-09 21:18 ` [PATCH 2/2] vfs: remove unused __d_splice_alias argument J. Bruce Fields
1 sibling, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2012-05-09 21:18 UTC (permalink / raw)
To: Al Viro
Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, J. Bruce Fields
From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
A directory should never have more than one dentry pointing to it.
But d_splice_alias() will add one if it finds a directory with an
already-existing non-DISCONNECTED dentry.
I can't find an obvious reproducer, but I also can't see what prevents
d_splice_alias() from encountering such a case.
It therefore seems safest to allow d_splice_alias to use any dentry it
finds.
(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.
That reproducer no longer exists, but I don't think there was anything
fundamentally incorrect about the vfs_rmdir() behavior there, so I think
the real fault was here in d_splice_alias().)
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
fs/dcache.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index b60ddc4..2434c1e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1606,9 +1606,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);
+ 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);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] vfs: remove unused __d_splice_alias argument
[not found] ` <1336598286-28636-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-05-09 21:18 ` [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases J. Bruce Fields
@ 2012-05-09 21:18 ` J. Bruce Fields
1 sibling, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2012-05-09 21:18 UTC (permalink / raw)
To: Al Viro
Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, J. Bruce Fields
From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Nobody sets want_disconn any more.
Reported-by: Peng Tao <bergwolf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
fs/dcache.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 2434c1e..8e34e91 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -649,8 +649,6 @@ EXPORT_SYMBOL(dget_parent);
/**
* d_find_alias - grab a hashed alias of inode
* @inode: inode in question
- * @want_discon: flag, used by d_splice_alias, to request
- * that only a DISCONNECTED alias be returned.
*
* If inode has a hashed alias, or is a directory and has any alias,
* acquire the reference to alias and return it. Otherwise return NULL.
@@ -659,10 +657,9 @@ EXPORT_SYMBOL(dget_parent);
* of a filesystem.
*
* If the inode has an IS_ROOT, DCACHE_DISCONNECTED alias, then prefer
- * any other hashed alias over that one unless @want_discon is set,
- * in which case only return an IS_ROOT, DCACHE_DISCONNECTED alias.
+ * any other hashed alias over that.
*/
-static struct dentry *__d_find_alias(struct inode *inode, int want_discon)
+static struct dentry *__d_find_alias(struct inode *inode)
{
struct dentry *alias, *discon_alias;
@@ -674,7 +671,7 @@ again:
if (IS_ROOT(alias) &&
(alias->d_flags & DCACHE_DISCONNECTED)) {
discon_alias = alias;
- } else if (!want_discon) {
+ } else {
__dget_dlock(alias);
spin_unlock(&alias->d_lock);
return alias;
@@ -705,7 +702,7 @@ struct dentry *d_find_alias(struct inode *inode)
if (!list_empty(&inode->i_dentry)) {
spin_lock(&inode->i_lock);
- de = __d_find_alias(inode, 0);
+ de = __d_find_alias(inode);
spin_unlock(&inode->i_lock);
}
return de;
@@ -2395,7 +2392,7 @@ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
struct dentry *alias;
/* Does an aliased dentry already exist? */
- alias = __d_find_alias(inode, 0);
+ alias = __d_find_alias(inode);
if (alias) {
actual = alias;
write_seqlock(&rename_lock);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases
@ 2012-05-23 22:05 J. Bruce Fields
[not found] ` <1337810746-16240-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: J. Bruce Fields @ 2012-05-23 22:05 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, linux-nfs, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
A directory should never have more than one dentry pointing to it.
But d_splice_alias() will add one if it finds a directory with an
already-existing non-DISCONNECTED dentry.
I can't find an obvious reproducer, but I also can't see what prevents
d_splice_alias() from encountering such a case.
It therefore seems safest to allow d_splice_alias to use any dentry it
finds.
(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.
That reproducer no longer exists, but I don't think there was anything
fundamentally incorrect about the vfs_rmdir() behavior there, so I think
the real fault was here in d_splice_alias().)
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/dcache.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index b60ddc4..2434c1e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1606,9 +1606,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);
+ 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);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases
[not found] ` <1337810746-16240-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2012-05-23 22:07 ` J. Bruce Fields
0 siblings, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2012-05-23 22:07 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Al Viro, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA
These two patches are a resend. I think they're appropriate for 3.5.
--b.
On Wed, May 23, 2012 at 06:05:45PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> A directory should never have more than one dentry pointing to it.
>
> But d_splice_alias() will add one if it finds a directory with an
> already-existing non-DISCONNECTED dentry.
>
> I can't find an obvious reproducer, but I also can't see what prevents
> d_splice_alias() from encountering such a case.
>
> It therefore seems safest to allow d_splice_alias to use any dentry it
> finds.
>
> (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.
>
> That reproducer no longer exists, but I don't think there was anything
> fundamentally incorrect about the vfs_rmdir() behavior there, so I think
> the real fault was here in d_splice_alias().)
>
> Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> fs/dcache.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index b60ddc4..2434c1e 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1606,9 +1606,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);
> + 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);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-23 22:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-09 21:18 DCACHE_DISCONNECTED patches J. Bruce Fields
[not found] ` <1336598286-28636-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-05-09 21:18 ` [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases J. Bruce Fields
2012-05-09 21:18 ` [PATCH 2/2] vfs: remove unused __d_splice_alias argument J. Bruce Fields
-- strict thread matches above, loose matches on Subject: below --
2012-05-23 22:05 [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases J. Bruce Fields
[not found] ` <1337810746-16240-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2012-05-23 22:07 ` J. Bruce Fields
2012-03-13 20:55 [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries J. Bruce Fields
2012-03-13 20:58 ` [PATCH 1/2] vfs: stop d_splice_alias creating directory aliases J. Bruce Fields
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).