ubifs, race between link and unlink/rename?

ubifs, race between link and unlink/rename?

[hooanon05]

Hello,

Is there a race condition in ubifs?
Here is a scenario.

Process A		Process B
----------------------+---------------------------
create("dirA/fileA"); |
unlink("dirA/fileA"); |	link("dirA/fileA", "dirB/fileB");
		      | unlink("dirB/fileB");
----------------------+---------------------------

In link(2), dirA->i_mutex is not held. So unlink("dirA/fileA") can run
concurrently (after lookup).
While ubifs acquires ubifs_inode->ui_mutex, it doesn't check i_nlink.
When unlink("dirA/fileA") wins ui_mutex race and link() loses,
- ubifs_unlink("dirA/fileA") will call ubifs_jnl_update/ubifs_add_orphan()
- ubifs_link() will operate the inode with i_nlink == 0 (finally it will
  be 1)
- ubifs_unlink("dirB/fileB") will call ubifs_add_orphan() again for the
  same inode
- and it will produce "orphaned twice" error.
- (ubifs_add_orphan() is called by ubifs_rename/ubifs_jnl_rename() too)

If this scenario is possible, it may happen in every FS.
To check i_nlink in vfs_link (like this) may be one option, but it might
be better to check in ubifs since ubifs_add_orphan() is for i_nlink == 0
and ubifs specific.

diff --git a/fs/namei.c b/fs/namei.c
index b207821..820c386 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2409,9 +2409,12 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
 	if (error)
 		return error;
 
+	error = -ENOENT;
 	mutex_lock(&inode->i_mutex);
-	DQUOT_INIT(dir);
-	error = dir->i_op->link(old_dentry, dir, new_dentry);
+	if (inode->i_nlink) {
+		DQUOT_INIT(dir);
+		error = dir->i_op->link(old_dentry, dir, new_dentry);
+	}
 	mutex_unlock(&inode->i_mutex);
 	if (!error)
 		fsnotify_link(dir, inode, new_dentry);


J. R. Okajima

Re: ubifs, race between link and unlink/rename?

[Adrian Hunter]

hooanon05@yahoo.co.jp wrote:
> Is there a race condition in ubifs?

Yes, it does look like it.  EXT3 and EXT4 explicitly check for nlink == 0
in their link functions.  UBIFS seems to have overlooked that check.

I guess some file systems do not have a problem with the race, which is why
VFS allows it.  We will patch UBIFS, but I cannot comment about changing
VFS.

Thank you for finding this :-)

> Here is a scenario.
> 
> Process A		Process B
> ----------------------+---------------------------
> create("dirA/fileA"); |
> unlink("dirA/fileA"); |	link("dirA/fileA", "dirB/fileB");
> 		      | unlink("dirB/fileB");
> ----------------------+---------------------------
> 
> In link(2), dirA->i_mutex is not held. So unlink("dirA/fileA") can run
> concurrently (after lookup).
> While ubifs acquires ubifs_inode->ui_mutex, it doesn't check i_nlink.
> When unlink("dirA/fileA") wins ui_mutex race and link() loses,
> - ubifs_unlink("dirA/fileA") will call ubifs_jnl_update/ubifs_add_orphan()
> - ubifs_link() will operate the inode with i_nlink == 0 (finally it will
>   be 1)
> - ubifs_unlink("dirB/fileB") will call ubifs_add_orphan() again for the
>   same inode
> - and it will produce "orphaned twice" error.
> - (ubifs_add_orphan() is called by ubifs_rename/ubifs_jnl_rename() too)
> 
> If this scenario is possible, it may happen in every FS.
> To check i_nlink in vfs_link (like this) may be one option, but it might
> be better to check in ubifs since ubifs_add_orphan() is for i_nlink == 0
> and ubifs specific.
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index b207821..820c386 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2409,9 +2409,12 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
>  	if (error)
>  		return error;
>  
> +	error = -ENOENT;
>  	mutex_lock(&inode->i_mutex);
> -	DQUOT_INIT(dir);
> -	error = dir->i_op->link(old_dentry, dir, new_dentry);
> +	if (inode->i_nlink) {
> +		DQUOT_INIT(dir);
> +		error = dir->i_op->link(old_dentry, dir, new_dentry);
> +	}
>  	mutex_unlock(&inode->i_mutex);
>  	if (!error)
>  		fsnotify_link(dir, inode, new_dentry);
> 
> 
> J. R. Okajima
> 

Re: ubifs, race between link and unlink/rename?

[Artem Bityutskiy]

On Thu, 2009-05-14 at 01:31 +0900, hooanon05@yahoo.co.jp wrote:
> diff --git a/fs/namei.c b/fs/namei.c
> index b207821..820c386 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2409,9 +2409,12 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
>  	if (error)
>  		return error;
>  
> +	error = -ENOENT;
>  	mutex_lock(&inode->i_mutex);
> -	DQUOT_INIT(dir);
> -	error = dir->i_op->link(old_dentry, dir, new_dentry);
> +	if (inode->i_nlink) {
> +		DQUOT_INIT(dir);
> +		error = dir->i_op->link(old_dentry, dir, new_dentry);
> +	}
>  	mutex_unlock(&inode->i_mutex);
>  	if (!error)
>  		fsnotify_link(dir, inode, new_dentry);
> 

Looks correct to me. I wonder what VFS guys think - should we fix UBIFS
or this can be done at the VFS layer?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

--
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: ubifs, race between link and unlink/rename?

[Artem Bityutskiy]

On Thu, 2009-05-14 at 01:31 +0900, hooanon05@yahoo.co.jp wrote:
> Hello,
> 
> Is there a race condition in ubifs?
> Here is a scenario.
> 
> Process A		Process B
> ----------------------+---------------------------
> create("dirA/fileA"); |
> unlink("dirA/fileA"); |	link("dirA/fileA", "dirB/fileB");
> 		      | unlink("dirB/fileB");
> ----------------------+---------------------------

From: Hunter Adrian <adrian.hunter@nokia.com>
Date: Thu, 14 May 2009 06:32:30 +0200
Subject: [PATCH] UBIFS: return error if link and unlink race

Consider a scenario when 'vfs_link(dirA/fileA)' and
'vfs_unlink(dirA/fileA, dirB/fileB)' race. 'vfs_link()' does not
lock 'dirA->i_mutex', so this is possible. Both of the functions
lock 'fileA->i_mutex' though. Suppose 'vfs_unlink()' wins, and takes
'fileA->i_mutex' mutex first. Suppose 'fileA->i_nlink' is 1. In this
case 'ubifs_unlink()' will drop the last reference, and put 'inodeA'
to the list of orphans. After this, 'vfs_link()' will link
'dirB/fileB' to 'inodeA'. Thir is a problem because, for example,
the subsequent 'vfs_unlink(dirB/fileB)' will add the same inode
to the list of orphans.

This problem was reported by J. R. Okajima <hooanon05@yahoo.co.jp>

[Artem: add more comments, amended commit message]

Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/dir.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index f55d523..552fb01 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -528,6 +528,25 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
 		inode->i_nlink, dir->i_ino);
 	ubifs_assert(mutex_is_locked(&dir->i_mutex));
 	ubifs_assert(mutex_is_locked(&inode->i_mutex));
+
+	/*
+	 * Return -ENOENT if we've raced with unlink and i_nlink is 0.  Doing
+	 * otherwise has the potential to corrupt the orphan inode list.
+	 *
+	 * Indeed, consider a scenario when 'vfs_link(dirA/fileA)' and
+	 * 'vfs_unlink(dirA/fileA, dirB/fileB)' race. 'vfs_link()' does not
+	 * lock 'dirA->i_mutex', so this is possible. Both of the functions
+	 * lock 'fileA->i_mutex' though. Suppose 'vfs_unlink()' wins, and takes
+	 * 'fileA->i_mutex' mutex first. Suppose 'fileA->i_nlink' is 1. In this
+	 * case 'ubifs_unlink()' will drop the last reference, and put 'inodeA'
+	 * to the list of orphans. After this, 'vfs_link()' will link
+	 * 'dirB/fileB' to 'inodeA'. This is a problem because, for example,
+	 * the subsequent 'vfs_unlink(dirB/fileB)' will add the same inode
+	 * to the list of orphans.
+	 */
+	 if (inode->i_nlink == 0)
+		 return -ENOENT;
+
 	err = dbg_check_synced_i_size(inode);
 	if (err)
 		return err;
-- 
1.6.0.6

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

--
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: ubifs, race between link and unlink/rename?

[Artem Bityutskiy]

On Fri, 2009-05-15 at 12:36 +0300, Artem Bityutskiy wrote:
> On Thu, 2009-05-14 at 01:31 +0900, hooanon05@yahoo.co.jp wrote:
> > Hello,
> > 
> > Is there a race condition in ubifs?
> > Here is a scenario.
> > 
> > Process A		Process B
> > ----------------------+---------------------------
> > create("dirA/fileA"); |
> > unlink("dirA/fileA"); |	link("dirA/fileA", "dirB/fileB");
> > 		      | unlink("dirB/fileB");
> > ----------------------+---------------------------
> 
> From: Hunter Adrian <adrian.hunter@nokia.com>
> Date: Thu, 14 May 2009 06:32:30 +0200
> Subject: [PATCH] UBIFS: return error if link and unlink race
> 
> Consider a scenario when 'vfs_link(dirA/fileA)' and
> 'vfs_unlink(dirA/fileA, dirB/fileB)' race. 'vfs_link()' does not
> lock 'dirA->i_mutex', so this is possible. Both of the functions
> lock 'fileA->i_mutex' though. Suppose 'vfs_unlink()' wins, and takes
> 'fileA->i_mutex' mutex first. Suppose 'fileA->i_nlink' is 1. In this
> case 'ubifs_unlink()' will drop the last reference, and put 'inodeA'
> to the list of orphans. After this, 'vfs_link()' will link
> 'dirB/fileB' to 'inodeA'. Thir is a problem because, for example,
> the subsequent 'vfs_unlink(dirB/fileB)' will add the same inode
> to the list of orphans.
> 
> This problem was reported by J. R. Okajima <hooanon05@yahoo.co.jp>
> 
> [Artem: add more comments, amended commit message]
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---

Pushing this patch to ubifs-2.6.git, thanks.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

--
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: ubifs, race between link and unlink/rename?

[hooanon05]

Artem Bityutskiy:
> > Consider a scenario when 'vfs_link(dirA/fileA)' and
> > 'vfs_unlink(dirA/fileA, dirB/fileB)' race. 'vfs_link()' does not
> > lock 'dirA->i_mutex', so this is possible. Both of the functions
	:::
> Pushing this patch to ubifs-2.6.git, thanks.

Ah, I am late.
We need one minor correction.


> @@ -528,6 +528,25 @@ static int ubifs_link(struct dentry *old_dentry, struct inode *dir,
>  		inode->i_nlink, dir->i_ino);
>  	ubifs_assert(mutex_is_locked(&dir->i_mutex));
>  	ubifs_assert(mutex_is_locked(&inode->i_mutex));
> +
> +	/*
> +	 * Return -ENOENT if we've raced with unlink and i_nlink is 0.  Doing
> +	 * otherwise has the potential to corrupt the orphan inode list.
> +	 *
> +	 * Indeed, consider a scenario when 'vfs_link(dirA/fileA)' and
> +	 * 'vfs_unlink(dirA/fileA, dirB/fileB)' race. 'vfs_link()' does not

This comment is confusing.
Need to swich vfs_unlink and vfs_link. :-)


J. R. Okajima