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

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

[Theodore Ts'o]

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" <tytso@mit.edu>
---
 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

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

[Andreas Dilger]

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" <tytso@mit.edu>
> ---
> 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

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

[Ted Ts'o]

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 <adilger@dilger.ca>
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 <adilger@dilger.ca>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

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,

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

[Jan Kara]

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

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

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

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

[-- Attachment #2: 0001-vfs-Avoid-creation-of-directory-loops-for-corrupted-.patch --]
[-- Type: text/x-patch, Size: 1231 bytes --]

>From 0715b656ac88ce1bb62800b14d99ef2e25c26d28 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
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 <sami.liedes@iki.fi>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 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

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

[J. Bruce Fields]

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 <jack@suse.cz>
> 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 <sami.liedes@iki.fi>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  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
> 

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

[Jan Kara]

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 <jack@suse.cz>
> > 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 <sami.liedes@iki.fi>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  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 <jack@suse.cz>
SUSE Labs, CR

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

[J. Bruce Fields]

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 <jack@suse.cz>
> > > 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 <sami.liedes@iki.fi>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  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 <jack@suse.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Kara]

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 <jack@suse.cz>
SUSE Labs, CR

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

[Greg KH]

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" <tytso@mit.edu>
> ---
>  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

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

[Ted Ts'o]

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" <tytso@mit.edu>
> > ---
> >  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

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

[Bernd Schubert]

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"<tytso@mit.edu>
> ---
>   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

Re: ext2 hang on (intentionally) corrupted filesystem

[Ted Ts'o]

On Wed, May 09, 2012 at 11:12:36PM +0200, Jan Kara wrote:
> > 1. wget http://sli.dy.fi/~sliedes/berserker/testcases/ext2.110.min.bz2
> > 2. mount ... /mnt -t ext2 -o errors=continue
> > 3. Do some operations; what I do (it's the rm that crashes):
> >   timeout 30 rm -rf /mnt/* >&/dev/null
> > 4. The rm task hangs
> > 
>   OK, you've changed '.' directory entry to a normal directory entry with a
> name 0x6e. I guess that has some potential in confusing something. Actually
> rm -rf does not reproduce the problem for me (it just complains about
> cyclic directory hierarchy) but trying to rmdir bad entry hangs the system
> - we try to grab i_mutex for the directory twice because the directory is
> it's own parent... That would be kind of hard to fix in VFS since once our
> directory structure contains a cycle, our locking protocol is no longer
> deadlock free. I'll see what we could do...

Just wanted to chime in that this crashes when the file system is
mounted using ext4; not surprising, since it's clearly a VFS issue.

The following proof-of-concept patch (see reply chained to this mail
message) fixes the problem for your test file system.  Al, what do you
think?  Is it worth it to define a new mechanism where we can pass
VFS-detected corruption down to the low-level file system?

						- Ted

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

[Theodore Ts'o]

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" <tytso@mit.edu>
---
 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