Ocfs2 devel - Jun 2013 - [PATCH] ocfs2: fix readonly issue in ocfs2_unlink()

While deleting a file with ocfs2_unlink(), there is a bug in this 
function. This bug will result in filesystem read-only.

After calling ocfs2_orphan_add(), the file which will be deleted 
is added into orphan dir. If ocfs2_delete_entry() fails, 
the file still exists in the parent dir. 
And this scenario introduces a conflict of metadata.

If a file is added into orphan dir, when we put inode of the file 
with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in 
ocfs2_remove_inode(), and then write back to disk. 

But as previously mentioned, the file still exists in the parent dir.
On other nodes, the file can be still accessed. When first read the file 
with ocfs2_read_blocks() from disk, It will check and avalidate inode 
using ocfs2_validate_inode_block(). 
So File system will be readonly because the inode is invalid.
In other words, the inode i_flags has been setted (~OCFS2_VALID_FL).


Signed-off-by: Younger Liu <younger.liu at huawei.com>
Signed-off-by: Jensen <shencanquan at huawei.com> 
---
 fs/ocfs2/namei.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index e3fce49..ddb4915 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir,
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0;
+	int child_locked = 0, is_unlinkable = 0;
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
@@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir,
 			mlog_errno(status);
 			goto leave;
 		}
+		is_unlinkable = 1;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));
@@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir,
 
 	fe = (struct ocfs2_dinode *) fe_bh->b_data;
 
-	if (inode_is_unlinkable(inode)) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
-		if (status < 0) {
-			mlog_errno(status);
-			goto leave;
-		}
-	}
-
 	/* delete the name from the parent dir */
 	status = ocfs2_delete_entry(handle, dir, &lookup);
 	if (status < 0) {
@@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir,
 		mlog_errno(status);
 		if (S_ISDIR(inode->i_mode))
 			inc_nlink(dir);
+		goto leave;
+	}
+
+	if (is_unlinkable) {
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
+					  &orphan_insert, orphan_dir);
+		if (status < 0)
+			mlog_errno(status);
 	}
 
 leave:
-- 
1.7.9.7

On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu <younger.liu at huawei.com>
wrote:
> While deleting a file with ocfs2_unlink(), there is a bug in this 
> function. This bug will result in filesystem read-only.
> 
> After calling ocfs2_orphan_add(), the file which will be deleted 
> is added into orphan dir. If ocfs2_delete_entry() fails, 
> the file still exists in the parent dir. 
> And this scenario introduces a conflict of metadata.
> 
> If a file is added into orphan dir, when we put inode of the file 
> with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in 
> ocfs2_remove_inode(), and then write back to disk. 
> 
> But as previously mentioned, the file still exists in the parent dir.
> On other nodes, the file can be still accessed. When first read the file 
> with ocfs2_read_blocks() from disk, It will check and avalidate inode 
> using ocfs2_validate_inode_block(). 
> So File system will be readonly because the inode is invalid.
> In other words, the inode i_flags has been setted (~OCFS2_VALID_FL).
> 
> ...
>
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir,
>  			struct dentry *dentry)
>  {
>  	int status;
> -	int child_locked = 0;
> +	int child_locked = 0, is_unlinkable = 0;
Please note that the surrounding code was carful to use the
one-definition-per-line convention.  That's a good convention - more
readable, less patch rejects during code evolution, leaves room for a
nice little comment.

Also, type `bool' would have been appropraite here.
>  	struct inode *inode = dentry->d_inode;
>  	struct inode *orphan_dir = NULL;
>  	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
> @@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir,
>  			mlog_errno(status);
>  			goto leave;
>  		}
> +		is_unlinkable = 1;
>  	}
>  
>  	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));
> @@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir,
>  
>  	fe = (struct ocfs2_dinode *) fe_bh->b_data;
>  
> -	if (inode_is_unlinkable(inode)) {
> -		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
> -					  &orphan_insert, orphan_dir);
> -		if (status < 0) {
> -			mlog_errno(status);
> -			goto leave;
> -		}
> -	}
> -
>  	/* delete the name from the parent dir */
>  	status = ocfs2_delete_entry(handle, dir, &lookup);
>  	if (status < 0) {
> @@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir,
>  		mlog_errno(status);
>  		if (S_ISDIR(inode->i_mode))
>  			inc_nlink(dir);
> +		goto leave;
> +	}
> +
> +	if (is_unlinkable) {
> +		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
> +					  &orphan_insert, orphan_dir);
> +		if (status < 0)
> +			mlog_errno(status);
>  	}
This is yet another ocfs2 function which reports the same error two
times.  ho hum.

Please review:

--- a/fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix
+++ a/fs/ocfs2/namei.c
@@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di
 			struct dentry *dentry)
 {
 	int status;
-	int child_locked = 0, is_unlinkable = 0;
+	int child_locked = 0;
+	bool is_unlinkable = false;
 	struct inode *inode = dentry->d_inode;
 	struct inode *orphan_dir = NULL;
 	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
@@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di
 			mlog_errno(status);
 			goto leave;
 		}
-		is_unlinkable = 1;
+		is_unlinkable = true;
 	}
 
 	handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb));
@@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di
 	}
 
 	if (is_unlinkable) {
-		status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name,
-					  &orphan_insert, orphan_dir);
+		status = ocfs2_orphan_add(osb, handle, inode, fe_bh,
+				orphan_name, &orphan_insert, orphan_dir);
 		if (status < 0)
 			mlog_errno(status);
 	}
_