[PATCH 0/4] Avoid deadlock when corrupted directory creates a loop

[PATCH 0/4] Avoid deadlock when corrupted directory creates a loop

[Jan Kara]

  Hello,

  Sami Liedes reported a problem that when a accessing a corrupted filesystem
the system deadlocks. The culprit is that the corruption has created a (self-)
loop in the directory hierarchy and thus we deadlock when trying to lock
i_mutex twice.

This patch set attempts at fixing the problem since it seems relatively
straightforward. We teach d_splice_alias() to fail when it would add the second
connected alias to a directory inode and then handle the failure in
filesystems. So far I've taugh ext? filesystems to handle the failure properly
but other filesystems should be similarly trivial. Just I'd like to hear
whether people agree to the general idea of the fix first.

								Honza

[PATCH 1/4] vfs: Avoid creation of directory loops for corrupted filesystems

[Jan Kara]

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

[PATCH 2/4] ext2: Handle error from d_splice_alias()

[Jan Kara]

When directory hiearchy is corrupted and contains cycles, d_splice_alias() can
fail. Handle the failure cleanly.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/namei.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index f663a67..d5f8aab 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -59,6 +59,7 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
 {
 	struct inode * inode;
 	ino_t ino;
+	struct dentry *ret;
 	
 	if (dentry->d_name.len > EXT2_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
@@ -74,7 +75,13 @@ static struct dentry *ext2_lookup(struct inode * dir, struct dentry *dentry, str
 			return ERR_PTR(-EIO);
 		}
 	}
-	return d_splice_alias(inode, dentry);
+	ret = d_splice_alias(inode, dentry);
+	if (IS_ERR(ret)) {
+		ext2_error(dir->i_sb, __func__, "directory #%lu corrupted",
+			   dir->i_ino);
+		iput(inode);
+	}
+	return ret;
 }
 
 struct dentry *ext2_get_parent(struct dentry *child)
-- 
1.7.1

[PATCH 3/4] ext3: Handle error from d_splice_alias()

[Jan Kara]

When directory hiearchy is corrupted and contains cycles, d_splice_alias() can
fail. Handle the failure cleanly.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/namei.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index eeb63df..faf09b7 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -1016,6 +1016,7 @@ static struct dentry *ext3_lookup(struct inode * dir, struct dentry *dentry, str
 	struct inode * inode;
 	struct ext3_dir_entry_2 * de;
 	struct buffer_head * bh;
+	struct dentry *ret;
 
 	if (dentry->d_name.len > EXT3_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
@@ -1038,7 +1039,13 @@ static struct dentry *ext3_lookup(struct inode * dir, struct dentry *dentry, str
 			return ERR_PTR(-EIO);
 		}
 	}
-	return d_splice_alias(inode, dentry);
+	ret = d_splice_alias(inode, dentry);
+	if (IS_ERR(ret)) {
+		ext3_error(dir->i_sb, __func__, "directory #%lu corrupted",
+			   dir->i_ino);
+		iput(inode);
+	}
+	return ret;
 }
 
 
-- 
1.7.1

[PATCH 4/4] ext4: Handle error from d_splice_alias()

[Jan Kara]

When directory hiearchy is corrupted and contains cycles, d_splice_alias() can
fail. Handle the failure cleanly.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/namei.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e2a3f4b..fc65520 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1024,6 +1024,7 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, stru
 	struct inode *inode;
 	struct ext4_dir_entry_2 *de;
 	struct buffer_head *bh;
+	struct dentry *ret;
 
 	if (dentry->d_name.len > EXT4_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
@@ -1045,7 +1046,12 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, stru
 			return ERR_PTR(-EIO);
 		}
 	}
-	return d_splice_alias(inode, dentry);
+	ret = d_splice_alias(inode, dentry);
+	if (IS_ERR(ret)) {
+		EXT4_ERROR_INODE(dir, "directory corrupted");
+		iput(inode);
+	}
+	return ret;
 }
 
 
-- 
1.7.1

[PATCH 5/4] exofs: Handle error from d_splice_alias()

[Boaz Harrosh]

When directory hierarchy is corrupted and contains cycles, d_splice_alias() can
fail. Handle the failure cleanly.

Identical/coppied from:
	ext2: Handle error from d_splice_alias()
	Signed-off-by: Jan Kara <jack@suse.cz>

[exofs is just yet another copy/paste of ext2 code]

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/namei.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/exofs/namei.c b/fs/exofs/namei.c
index fc7161d..82de452 100644
--- a/fs/exofs/namei.c
+++ b/fs/exofs/namei.c
@@ -50,13 +50,19 @@ static struct dentry *exofs_lookup(struct inode *dir, struct dentry *dentry,
 {
 	struct inode *inode;
 	ino_t ino;
+	struct dentry *ret;
 
 	if (dentry->d_name.len > EXOFS_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
 	ino = exofs_inode_by_name(dir, dentry);
 	inode = ino ? exofs_iget(dir->i_sb, ino) : NULL;
-	return d_splice_alias(inode, dentry);
+	ret = d_splice_alias(inode, dentry);
+	if (IS_ERR(ret)) {
+		EXOFS_ERR("directory #%lu corrupted", dir->i_ino);
+		iput(inode);
+	}
+	return ret;
 }
 
 static int exofs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
-- 
1.7.10.2.677.gb6bc67f

Re: [PATCH 5/4] exofs: Handle error from d_splice_alias()

[Boaz Harrosh]

On 05/30/2012 01:39 PM, Boaz Harrosh wrote:

> 
> When directory hierarchy is corrupted and contains cycles, d_splice_alias() can
> fail. Handle the failure cleanly.
> 
> Identical/coppied from:
> 	ext2: Handle error from d_splice_alias()
> 	Signed-off-by: Jan Kara <jack@suse.cz>
> 
> [exofs is just yet another copy/paste of ext2 code]
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>


Please carry in your tree, since it's dependent on
[patch 1/4]

Thanks
Boaz

> ---
>  fs/exofs/namei.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/exofs/namei.c b/fs/exofs/namei.c
> index fc7161d..82de452 100644
> --- a/fs/exofs/namei.c
> +++ b/fs/exofs/namei.c
> @@ -50,13 +50,19 @@ static struct dentry *exofs_lookup(struct inode *dir, struct dentry *dentry,
>  {
>  	struct inode *inode;
>  	ino_t ino;
> +	struct dentry *ret;
>  
>  	if (dentry->d_name.len > EXOFS_NAME_LEN)
>  		return ERR_PTR(-ENAMETOOLONG);
>  
>  	ino = exofs_inode_by_name(dir, dentry);
>  	inode = ino ? exofs_iget(dir->i_sb, ino) : NULL;
> -	return d_splice_alias(inode, dentry);
> +	ret = d_splice_alias(inode, dentry);
> +	if (IS_ERR(ret)) {
> +		EXOFS_ERR("directory #%lu corrupted", dir->i_ino);
> +		iput(inode);
> +	}
> +	return ret;
>  }
>  
>  static int exofs_create(struct inode *dir, struct dentry *dentry, umode_t mode,

Re: [PATCH 5/4] exofs: Handle error from d_splice_alias()

[Al Viro]

On Wed, May 30, 2012 at 01:39:08PM +0300, Boaz Harrosh wrote:
> +	ret = d_splice_alias(inode, dentry);
> +	if (IS_ERR(ret)) {
> +		EXOFS_ERR("directory #%lu corrupted", dir->i_ino);
> +		iput(inode);

That's a bloody wrong interface.  If you add d_splice_alias() failure
exit like that, do iput() *there*.  Requiring every caller to deal with
failure exit cleanups like that is the recipe for recurring bugs.
Don't Do That.

Re: [PATCH 5/4] exofs: Handle error from d_splice_alias()

[Boaz Harrosh]

On 06/09/2012 12:59 AM, Al Viro wrote:

> On Wed, May 30, 2012 at 01:39:08PM +0300, Boaz Harrosh wrote:
>> +	ret = d_splice_alias(inode, dentry);
>> +	if (IS_ERR(ret)) {
>> +		EXOFS_ERR("directory #%lu corrupted", dir->i_ino);
>> +		iput(inode);
> 
> That's a bloody wrong interface.  If you add d_splice_alias() failure
> exit like that, do iput() *there*.  Requiring every caller to deal with
> failure exit cleanups like that is the recipe for recurring bugs.
> Don't Do That.


I agree. Thanks.

My point being that please any changes made to ext2, in this area please also
apply to exofs, since it is just another copy/paste of ext2. I'll ACK any
which way you guys decide to properly go with, as part of the VFS changes.

Thanks
Boaz

Re: [PATCH 5/4] exofs: Handle error from d_splice_alias()

[Ted Ts'o]

On Mon, Jun 11, 2012 at 06:41:30PM +0300, Boaz Harrosh wrote:
> 
> My point being that please any changes made to ext2, in this area please also
> apply to exofs, since it is just another copy/paste of ext2. I'll ACK any
> which way you guys decide to properly go with, as part of the VFS changes.

Well, I already have this quick and dirty fix to address the problem
in ext4.  See commit 7e936b7372.  If we need to make changes to all of
the file systems to accomodate some new VFS abstraction, it might be
worth considering whether it's easier/simpler to just put in a quick
check like I did for ext4 (just so I could plug the security hole[1]
quickly).

[1] It's a denial of service attack for kiosks that do automounts of
USB sticks; granted, it's not that big a of a security deal, but some
people care about such things.

Of course, if the new/changed VFS abstraction solves other problems,
that's cool, but if not, sometimes a simple brute force check is
better than something complicated if elegant.  :-)

      	    	     	    	 	   - Ted

Re: [PATCH 5/4] exofs: Handle error from d_splice_alias()

[Jan Kara]

On Mon 11-06-12 15:01:08, Ted Tso wrote:
> On Mon, Jun 11, 2012 at 06:41:30PM +0300, Boaz Harrosh wrote:
> > 
> > My point being that please any changes made to ext2, in this area please also
> > apply to exofs, since it is just another copy/paste of ext2. I'll ACK any
> > which way you guys decide to properly go with, as part of the VFS changes.
> 
> Well, I already have this quick and dirty fix to address the problem
> in ext4.  See commit 7e936b7372.  If we need to make changes to all of
> the file systems to accomodate some new VFS abstraction, it might be
> worth considering whether it's easier/simpler to just put in a quick
> check like I did for ext4 (just so I could plug the security hole[1]
> quickly).
> 
> [1] It's a denial of service attack for kiosks that do automounts of
> USB sticks; granted, it's not that big a of a security deal, but some
> people care about such things.
> 
> Of course, if the new/changed VFS abstraction solves other problems,
> that's cool, but if not, sometimes a simple brute force check is
> better than something complicated if elegant.  :-)
  I think that fix in ext4 is fine. Just you don't catch the situation when
the directory entry points e.g. to a parent and that's deadlockable
trivially as well. Even if it points to some unrelated directory, you can
easily deadlock rename which tries to lock both directories. So I attempted
for a fix in VFS because that's the only place having enough information to
be able to tell whether you are creating directory hardlink or not.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR