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

[Adrian Hunter <adrian.hunter@nokia.com>]

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
>