2012-05-28 17:33:44

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

If we rmdir a directory which is a hard link to '.', we will deadlock
trying to grab the directory's i_mutex. Check for this condition and
return EINVAL, which is what we return if the user attempts to rmdir
"/foo/bar/."

Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/namei.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 0062dd1..081f872 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2774,6 +2774,17 @@ static long do_rmdir(int dfd, const char __user *pathname)
error = -ENOENT;
goto exit3;
}
+ if (nd.path.dentry->d_inode == dentry->d_inode) {
+ /*
+ * Corrupt file system where there is a symlink to
+ * '.'; treat it as if we are trying to rmdir '.'
+ *
+ * XXX Should we call into the low-level file system
+ * to request that the file system be marked corrupt?
+ */
+ error = -EINVAL;
+ goto exit3;
+ }
error = mnt_want_write(nd.path.mnt);
if (error)
goto exit3;
--
1.7.10.2.552.gaa3bb87



2012-05-28 20:29:06

by Andreas Dilger

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On 2012-05-28, at 11:33 AM, Theodore Ts'o wrote:
> If we rmdir a directory which is a hard link to '.', we will deadlock
> trying to grab the directory's i_mutex. Check for this condition and
> return EINVAL, which is what we return if the user attempts to rmdir
> "/foo/bar/."
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> fs/namei.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 0062dd1..081f872 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2774,6 +2774,17 @@ static long do_rmdir(int dfd, const char __user *pathname)
> error = -ENOENT;
> goto exit3;
> }
> + if (nd.path.dentry->d_inode == dentry->d_inode) {
> + /*
> + * Corrupt file system where there is a symlink to
> + * '.'; treat it as if we are trying to rmdir '.'
> + *
> + * XXX Should we call into the low-level file system
> + * to request that the file system be marked corrupt?
> + */
> + error = -EINVAL;
> + goto exit3;
> + }
> error = mnt_want_write(nd.path.mnt);
> if (error)
> goto exit3;

This patch is good from the POV of covering all filesystems, and
avoiding the deadlock at the dcache level. It would be possible to
detect this problem in the filesystem itself during lookup, before
the bad link got into the dcache itself. Something like:

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 349d7b3..4a9c99d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1037,6 +1037,12 @@ static struct dentry *ext4_lookup(struct inode
EXT4_ERROR_INODE(dir, "bad inode number: %u", ino);
return ERR_PTR(-EIO);
}
+ if (ino == dir->i_ino) {
+ EXT4_ERROR_INODE(dir, "'%.*s' linked to parent dir",
+ dentry->d_name.len,
+ dentry->d_name.name);
+ return ERR_PTR(-EIO);
+ }
inode = ext4_iget(dir->i_sb, ino);
if (inode == ERR_PTR(-ESTALE)) {
EXT4_ERROR_INODE(dir,

Though -EIO could be replaced with -EBADF or -ELOOP, or something else.


Cheers, Andreas






2012-05-28 21:05:11

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> This patch is good from the POV of covering all filesystems, and
> avoiding the deadlock at the dcache level. It would be possible to
> detect this problem in the filesystem itself during lookup, before
> the bad link got into the dcache itself. Something like:

I like that as a solution for detecting the problem in ext4. As you
say, it's still an issue for other file systems, and so the patch I
proposed is still probably a good idea for the VFS. But this way ext4
(and ext3 when Jan backports it) will be able to detect the problem
and mark the file system as being corrupted.

Andreas, are you ok with the Signed-off-by? I gave you credit since
the patch is yours... (and do you want me to use the dilger.ca or the
whamcloud.com domain)?

Regards,

- Ted

commit bfd0ca03af12fa1dc439b57f65828dde2e7530e2
Author: Andreas Dilger <[email protected]>
Date: Mon May 28 17:02:25 2012 -0400

ext4: disallow hard-linked directory in ext4_lookup

A hard-linked directory to its parent can cause the VFS to deadlock,
and is a sign of a corrupted file system. So detect this case in
ext4_lookup(), before the rmdir() lockup scenario can take place.

Signed-off-by: Andreas Dilger <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index a9fd5f4..5f4a030 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1330,6 +1330,12 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, stru
EXT4_ERROR_INODE(dir, "bad inode number: %u", ino);
return ERR_PTR(-EIO);
}
+ if (ino == dir->i_ino) {
+ EXT4_ERROR_INODE(dir, "'%.*s' linked to parent dir",
+ dentry->d_name.len,
+ dentry->d_name.name);
+ return ERR_PTR(-EIO);
+ }
inode = ext4_iget(dir->i_sb, ino);
if (inode == ERR_PTR(-ESTALE)) {
EXT4_ERROR_INODE(dir,

2012-05-29 08:21:44

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Mon, May 28, 2012 at 01:33:42PM -0400, Theodore Ts'o wrote:
> If we rmdir a directory which is a hard link to '.', we will deadlock
> trying to grab the directory's i_mutex. Check for this condition and
> return EINVAL, which is what we return if the user attempts to rmdir
> "/foo/bar/."
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> fs/namei.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)

Any reason why you didn't also tag this for the stable kernel releases?

thanks,

greg k-h

2012-05-29 11:26:10

by Bernd Schubert

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On 05/28/2012 07:33 PM, Theodore Ts'o wrote:
> If we rmdir a directory which is a hard link to '.', we will deadlock
> trying to grab the directory's i_mutex. Check for this condition and
> return EINVAL, which is what we return if the user attempts to rmdir
> "/foo/bar/."
>
> Signed-off-by: "Theodore Ts'o"<[email protected]>
> ---
> fs/namei.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 0062dd1..081f872 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2774,6 +2774,17 @@ static long do_rmdir(int dfd, const char __user *pathname)
> error = -ENOENT;
> goto exit3;
> }
> + if (nd.path.dentry->d_inode == dentry->d_inode) {

Shouldn't this be tagged as unlikely()?

> + /*
> + * Corrupt file system where there is a symlink to
> + * '.'; treat it as if we are trying to rmdir '.'
> + *
> + * XXX Should we call into the low-level file system
> + * to request that the file system be marked corrupt?
> + */
> + error = -EINVAL;
> + goto exit3;
> + }
> error = mnt_want_write(nd.path.mnt);
> if (error)
> goto exit3;

Thanks,
Bernd

2012-05-29 12:18:38

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Tue, May 29, 2012 at 05:21:44PM +0900, Greg KH wrote:
> On Mon, May 28, 2012 at 01:33:42PM -0400, Theodore Ts'o wrote:
> > If we rmdir a directory which is a hard link to '.', we will deadlock
> > trying to grab the directory's i_mutex. Check for this condition and
> > return EINVAL, which is what we return if the user attempts to rmdir
> > "/foo/bar/."
> >
> > Signed-off-by: "Theodore Ts'o" <[email protected]>
> > ---
> > fs/namei.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
>
> Any reason why you didn't also tag this for the stable kernel releases?

Because I was sending it out for comment; I wanted to get Al's opinion
how to handle whether we needed to inform the low-level file system
about the corruption. I should have added an RFC to the subject line,
sorry.

Andrea's solution of detecting the corruption is lookup seems like the
right answer, so at this point, I'll remove the XXX comment, and add
the unlikely() annotation to the if statement, and resubmit with a Cc:
stable.

- Ted

2012-05-29 19:50:23

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Mon 28-05-12 17:05:11, Ted Tso wrote:
> On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> > This patch is good from the POV of covering all filesystems, and
> > avoiding the deadlock at the dcache level. It would be possible to
> > detect this problem in the filesystem itself during lookup, before
> > the bad link got into the dcache itself. Something like:
>
> I like that as a solution for detecting the problem in ext4. As you
> say, it's still an issue for other file systems, and so the patch I
> proposed is still probably a good idea for the VFS. But this way ext4
> (and ext3 when Jan backports it) will be able to detect the problem
> and mark the file system as being corrupted.
Actually, I think there's even better way. d_splice_alias() can rather
easily detect the problem and report it to filesystem. The advantage is
that the check in d_splice_alias() can catch any "hardlinks" to
directories, not just self loops. The patch is attached, I also have
corresponding handling written for ext? filesystems but that's trivial.
I'll post the whole series to Al to have a look.

Honza


2012-05-29 20:08:56

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Tue 29-05-12 21:50:19, Jan Kara wrote:
> On Mon 28-05-12 17:05:11, Ted Tso wrote:
> > On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> > > This patch is good from the POV of covering all filesystems, and
> > > avoiding the deadlock at the dcache level. It would be possible to
> > > detect this problem in the filesystem itself during lookup, before
> > > the bad link got into the dcache itself. Something like:
> >
> > I like that as a solution for detecting the problem in ext4. As you
> > say, it's still an issue for other file systems, and so the patch I
> > proposed is still probably a good idea for the VFS. But this way ext4
> > (and ext3 when Jan backports it) will be able to detect the problem
> > and mark the file system as being corrupted.
> Actually, I think there's even better way. d_splice_alias() can rather
> easily detect the problem and report it to filesystem. The advantage is
> that the check in d_splice_alias() can catch any "hardlinks" to
> directories, not just self loops. The patch is attached, I also have
> corresponding handling written for ext? filesystems but that's trivial.
> I'll post the whole series to Al to have a look.
And now with the attachment. Sorry.

Honza


Attachments:
(No filename) (1.22 kB)
0001-vfs-Avoid-creation-of-directory-loops-for-corrupted-.patch (1.20 kB)
Download all attachments

2012-05-30 17:37:18

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Tue, May 29, 2012 at 10:08:56PM +0200, Jan Kara wrote:
> On Tue 29-05-12 21:50:19, Jan Kara wrote:
> > On Mon 28-05-12 17:05:11, Ted Tso wrote:
> > > On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> > > > This patch is good from the POV of covering all filesystems, and
> > > > avoiding the deadlock at the dcache level. It would be possible to
> > > > detect this problem in the filesystem itself during lookup, before
> > > > the bad link got into the dcache itself. Something like:
> > >
> > > I like that as a solution for detecting the problem in ext4. As you
> > > say, it's still an issue for other file systems, and so the patch I
> > > proposed is still probably a good idea for the VFS. But this way ext4
> > > (and ext3 when Jan backports it) will be able to detect the problem
> > > and mark the file system as being corrupted.
> > Actually, I think there's even better way. d_splice_alias() can rather
> > easily detect the problem and report it to filesystem. The advantage is
> > that the check in d_splice_alias() can catch any "hardlinks" to
> > directories, not just self loops. The patch is attached, I also have
> > corresponding handling written for ext? filesystems but that's trivial.
> > I'll post the whole series to Al to have a look.
> And now with the attachment. Sorry.

Well, my understanding of d_splice_alias is that it should just return
the existing dentry instead of failing. (It does that now for
DISCONNECTED dentries, but I don't understand why they're special.)
So that's what:

http://git.kernel.org/?p=linux/kernel/git/viro/vfs.git;a=commit;h=9d345b3217b384813680901d42eae3fb380b9f77

does.

--b.

>
> Honza

> >From 0715b656ac88ce1bb62800b14d99ef2e25c26d28 Mon Sep 17 00:00:00 2001
> From: Jan Kara <[email protected]>
> Date: Tue, 29 May 2012 21:19:01 +0200
> Subject: [PATCH 1/4] vfs: Avoid creation of directory loops for corrupted filesystems
>
> When a directory hierarchy is corrupted (e. g. due to a bit flip on the media),
> it can happen that it contains loops of directories. That creates possibilities
> for deadlock when locking directories.
>
> Fix the problem by checking in d_splice_alias() that when we splice a
> directory, it does not have any other connected alias.
>
> Reported-by: Sami Liedes <[email protected]>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/dcache.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 4435d8b..ca31a1e 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1658,6 +1658,10 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
> d_move(new, dentry);
> iput(inode);
> } else {
> + if (unlikely(!list_empty(&inode->i_dentry))) {
> + spin_unlock(&inode->i_lock);
> + return ERR_PTR(-EIO);
> + }
> /* already taking inode->i_lock, so d_add() by hand */
> __d_instantiate(dentry, inode);
> spin_unlock(&inode->i_lock);
> --
> 1.7.1
>


2012-05-30 20:13:09

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Wed 30-05-12 13:37:09, J. Bruce Fields wrote:
> On Tue, May 29, 2012 at 10:08:56PM +0200, Jan Kara wrote:
> > On Tue 29-05-12 21:50:19, Jan Kara wrote:
> > > On Mon 28-05-12 17:05:11, Ted Tso wrote:
> > > > On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> > > > > This patch is good from the POV of covering all filesystems, and
> > > > > avoiding the deadlock at the dcache level. It would be possible to
> > > > > detect this problem in the filesystem itself during lookup, before
> > > > > the bad link got into the dcache itself. Something like:
> > > >
> > > > I like that as a solution for detecting the problem in ext4. As you
> > > > say, it's still an issue for other file systems, and so the patch I
> > > > proposed is still probably a good idea for the VFS. But this way ext4
> > > > (and ext3 when Jan backports it) will be able to detect the problem
> > > > and mark the file system as being corrupted.
> > > Actually, I think there's even better way. d_splice_alias() can rather
> > > easily detect the problem and report it to filesystem. The advantage is
> > > that the check in d_splice_alias() can catch any "hardlinks" to
> > > directories, not just self loops. The patch is attached, I also have
> > > corresponding handling written for ext? filesystems but that's trivial.
> > > I'll post the whole series to Al to have a look.
> > And now with the attachment. Sorry.
>
> Well, my understanding of d_splice_alias is that it should just return
> the existing dentry instead of failing. (It does that now for
> DISCONNECTED dentries, but I don't understand why they're special.)
> So that's what:
>
> http://git.kernel.org/?p=linux/kernel/git/viro/vfs.git;a=commit;h=9d345b3217b384813680901d42eae3fb380b9f77
>
> does.
Thanks for the pointer. In the case I tried to solve, returning the
existing dentry will solve the deadlocks, just user won't be warned that
the filesystem is corrupted. Since you seem to describe a valid case where
we can spot other !DISCONNECTED dentry of a directory, I guess we have no
other choice than using your approach.

We could do some sanity checks in ->lookup method (like Andreas suggested)
but they are not that powerful as a check in d_splice_alias() can be. But
what can one do...

Honza


> > >From 0715b656ac88ce1bb62800b14d99ef2e25c26d28 Mon Sep 17 00:00:00 2001
> > From: Jan Kara <[email protected]>
> > Date: Tue, 29 May 2012 21:19:01 +0200
> > Subject: [PATCH 1/4] vfs: Avoid creation of directory loops for corrupted filesystems
> >
> > When a directory hierarchy is corrupted (e. g. due to a bit flip on the media),
> > it can happen that it contains loops of directories. That creates possibilities
> > for deadlock when locking directories.
> >
> > Fix the problem by checking in d_splice_alias() that when we splice a
> > directory, it does not have any other connected alias.
> >
> > Reported-by: Sami Liedes <[email protected]>
> > Signed-off-by: Jan Kara <[email protected]>
> > ---
> > fs/dcache.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 4435d8b..ca31a1e 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1658,6 +1658,10 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
> > d_move(new, dentry);
> > iput(inode);
> > } else {
> > + if (unlikely(!list_empty(&inode->i_dentry))) {
> > + spin_unlock(&inode->i_lock);
> > + return ERR_PTR(-EIO);
> > + }
> > /* already taking inode->i_lock, so d_add() by hand */
> > __d_instantiate(dentry, inode);
> > spin_unlock(&inode->i_lock);
> > --
> > 1.7.1
> >
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2012-06-18 21:19:36

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Wed, May 30, 2012 at 10:12:57PM +0200, Jan Kara wrote:
> On Wed 30-05-12 13:37:09, J. Bruce Fields wrote:
> > On Tue, May 29, 2012 at 10:08:56PM +0200, Jan Kara wrote:
> > > On Tue 29-05-12 21:50:19, Jan Kara wrote:
> > > > On Mon 28-05-12 17:05:11, Ted Tso wrote:
> > > > > On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> > > > > > This patch is good from the POV of covering all filesystems, and
> > > > > > avoiding the deadlock at the dcache level. It would be possible to
> > > > > > detect this problem in the filesystem itself during lookup, before
> > > > > > the bad link got into the dcache itself. Something like:
> > > > >
> > > > > I like that as a solution for detecting the problem in ext4. As you
> > > > > say, it's still an issue for other file systems, and so the patch I
> > > > > proposed is still probably a good idea for the VFS. But this way ext4
> > > > > (and ext3 when Jan backports it) will be able to detect the problem
> > > > > and mark the file system as being corrupted.
> > > > Actually, I think there's even better way. d_splice_alias() can rather
> > > > easily detect the problem and report it to filesystem. The advantage is
> > > > that the check in d_splice_alias() can catch any "hardlinks" to
> > > > directories, not just self loops. The patch is attached, I also have
> > > > corresponding handling written for ext? filesystems but that's trivial.
> > > > I'll post the whole series to Al to have a look.
> > > And now with the attachment. Sorry.
> >
> > Well, my understanding of d_splice_alias is that it should just return
> > the existing dentry instead of failing. (It does that now for
> > DISCONNECTED dentries, but I don't understand why they're special.)
> > So that's what:
> >
> > http://git.kernel.org/?p=linux/kernel/git/viro/vfs.git;a=commit;h=9d345b3217b384813680901d42eae3fb380b9f77
> >
> > does.
> Thanks for the pointer. In the case I tried to solve, returning the
> existing dentry will solve the deadlocks, just user won't be warned that
> the filesystem is corrupted. Since you seem to describe a valid case where
> we can spot other !DISCONNECTED dentry of a directory, I guess we have no
> other choice than using your approach.

But my patch got reverted, on suspicion that it was either wrong or
covering up some other problem:

http://marc.info/?l=linux-fsdevel&m=133917767003505&w=2

... which an approach like yours might help at least find? So maybe
it's worth another try.

--b.

>
> We could do some sanity checks in ->lookup method (like Andreas suggested)
> but they are not that powerful as a check in d_splice_alias() can be. But
> what can one do...
>
> Honza
>
>
> > > >From 0715b656ac88ce1bb62800b14d99ef2e25c26d28 Mon Sep 17 00:00:00 2001
> > > From: Jan Kara <[email protected]>
> > > Date: Tue, 29 May 2012 21:19:01 +0200
> > > Subject: [PATCH 1/4] vfs: Avoid creation of directory loops for corrupted filesystems
> > >
> > > When a directory hierarchy is corrupted (e. g. due to a bit flip on the media),
> > > it can happen that it contains loops of directories. That creates possibilities
> > > for deadlock when locking directories.
> > >
> > > Fix the problem by checking in d_splice_alias() that when we splice a
> > > directory, it does not have any other connected alias.
> > >
> > > Reported-by: Sami Liedes <[email protected]>
> > > Signed-off-by: Jan Kara <[email protected]>
> > > ---
> > > fs/dcache.c | 4 ++++
> > > 1 files changed, 4 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > index 4435d8b..ca31a1e 100644
> > > --- a/fs/dcache.c
> > > +++ b/fs/dcache.c
> > > @@ -1658,6 +1658,10 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
> > > d_move(new, dentry);
> > > iput(inode);
> > > } else {
> > > + if (unlikely(!list_empty(&inode->i_dentry))) {
> > > + spin_unlock(&inode->i_lock);
> > > + return ERR_PTR(-EIO);
> > > + }
> > > /* already taking inode->i_lock, so d_add() by hand */
> > > __d_instantiate(dentry, inode);
> > > spin_unlock(&inode->i_lock);
> > > --
> > > 1.7.1
> > >
> >
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2012-06-20 09:57:06

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH] vfs: avoid hang caused by attempting to rmdir an invalid file system

On Mon 18-06-12 17:19:30, J. Bruce Fields wrote:
> On Wed, May 30, 2012 at 10:12:57PM +0200, Jan Kara wrote:
> > On Wed 30-05-12 13:37:09, J. Bruce Fields wrote:
> > > On Tue, May 29, 2012 at 10:08:56PM +0200, Jan Kara wrote:
> > > > On Tue 29-05-12 21:50:19, Jan Kara wrote:
> > > > > On Mon 28-05-12 17:05:11, Ted Tso wrote:
> > > > > > On Mon, May 28, 2012 at 02:29:05PM -0600, Andreas Dilger wrote:
> > > > > > > This patch is good from the POV of covering all filesystems, and
> > > > > > > avoiding the deadlock at the dcache level. It would be possible to
> > > > > > > detect this problem in the filesystem itself during lookup, before
> > > > > > > the bad link got into the dcache itself. Something like:
> > > > > >
> > > > > > I like that as a solution for detecting the problem in ext4. As you
> > > > > > say, it's still an issue for other file systems, and so the patch I
> > > > > > proposed is still probably a good idea for the VFS. But this way ext4
> > > > > > (and ext3 when Jan backports it) will be able to detect the problem
> > > > > > and mark the file system as being corrupted.
> > > > > Actually, I think there's even better way. d_splice_alias() can rather
> > > > > easily detect the problem and report it to filesystem. The advantage is
> > > > > that the check in d_splice_alias() can catch any "hardlinks" to
> > > > > directories, not just self loops. The patch is attached, I also have
> > > > > corresponding handling written for ext? filesystems but that's trivial.
> > > > > I'll post the whole series to Al to have a look.
> > > > And now with the attachment. Sorry.
> > >
> > > Well, my understanding of d_splice_alias is that it should just return
> > > the existing dentry instead of failing. (It does that now for
> > > DISCONNECTED dentries, but I don't understand why they're special.)
> > > So that's what:
> > >
> > > http://git.kernel.org/?p=linux/kernel/git/viro/vfs.git;a=commit;h=9d345b3217b384813680901d42eae3fb380b9f77
> > >
> > > does.
> > Thanks for the pointer. In the case I tried to solve, returning the
> > existing dentry will solve the deadlocks, just user won't be warned that
> > the filesystem is corrupted. Since you seem to describe a valid case where
> > we can spot other !DISCONNECTED dentry of a directory, I guess we have no
> > other choice than using your approach.
>
> But my patch got reverted, on suspicion that it was either wrong or
> covering up some other problem:
>
> http://marc.info/?l=linux-fsdevel&m=133917767003505&w=2
>
> ... which an approach like yours might help at least find? So maybe
> it's worth another try.
Yeah, I'll rebase and resubmit those patches (plus fixup error handling
as Al suggested) today or tomorrow.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR