Ocfs2 devel - May 2013 - [PATCH v2] ocfs2: should not use le32_add_cpu to set ocfs2_dinode i_flags

If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
corresponding flag corrupted. So we should change it to bitwise and/or
operation.

Signed-off-by: Joseph Qi <joseph.qi at huawei.com>
---
 fs/ocfs2/namei.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 04ee1b5..f53471d 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,

 	fe->i_last_eb_blk = 0;
 	strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
-	le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
+	fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
 	fe->i_atime = fe->i_ctime = fe->i_mtime  	
cpu_to_le64(CURRENT_TIME.tv_sec);
 	fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec @@ -2044,7
+2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
 		goto leave;
 	}

-	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
+	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
 	OCFS2_I(inode)->ip_flags &= ~OCFS2_INODE_SKIP_ORPHAN_DIR;

 	/* Record which orphan dir our inode now resides
@@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
 	}

 	di = (struct ocfs2_dinode *)di_bh->b_data;
-	le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
+	di->i_flags &= ~cpu_to_le32(OCFS2_ORPHANED_FL);
 	di->i_orphaned_slot = 0;
 	set_nlink(inode, 1);
 	ocfs2_set_links_count(di, inode->i_nlink);
-- 
1.7.9.7

On 05/30/2013 12:49 PM, Joseph Qi wrote:
> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
> corresponding flag corrupted. So we should change it to bitwise and/or
> operation.
> 
> Signed-off-by: Joseph Qi <joseph.qi at huawei.com>
Reviewed-by: Jie Liu <jeff.liu at oracle.com>
> ---
>  fs/ocfs2/namei.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 04ee1b5..f53471d 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir,
> 
>  	fe->i_last_eb_blk = 0;
>  	strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE);
> -	le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL);
> +	fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL);
>  	fe->i_atime = fe->i_ctime = fe->i_mtime >  	
cpu_to_le64(CURRENT_TIME.tv_sec);
>  	fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec > @@
-2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>  		goto leave;
>  	}
> 
> -	le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL);
> +	fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>  	OCFS2_I(inode)->ip_flags &= ~OCFS2_INODE_SKIP_ORPHAN_DIR;
> 
>  	/* Record which orphan dir our inode now resides
> @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
>  	}
> 
>  	di = (struct ocfs2_dinode *)di_bh->b_data;
> -	le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL);
> +	di->i_flags &= ~cpu_to_le32(OCFS2_ORPHANED_FL);
>  	di->i_orphaned_slot = 0;
>  	set_nlink(inode, 1);
>  	ocfs2_set_links_count(di, inode->i_nlink);

On Thu, 30 May 2013 12:49:46 +0800 Joseph Qi <joseph.qi at huawei.com>
wrote:
> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
> corresponding flag corrupted. So we should change it to bitwise and/or
> operation.
> 
The usual question: what is the end-user impact of the bug which was
just fixed?

For the usual reason: so I and others can decide which kernel versions
need the fix.

Thanks.

Hi Andrew,

Sorry for the late response because I just returned from the LinuxCon/Japan.

I'm afraid that Joesph can not follow this thread up in time since today is
the
weekend in China.  So please let me answer your question on behalf of Joesph
as a quick response.

On 06/01/2013 06:25 AM, Andrew Morton wrote:
> On Thu, 30 May 2013 12:49:46 +0800 Joseph Qi <joseph.qi at
huawei.com> wrote:
> 
>> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the
>> corresponding flag corrupted. So we should change it to bitwise and/or
>> operation.
>>
> 
> The usual question: what is the end-user impact of the bug which was
> just fixed?
This patch can be treated as a trivial fix.  Yes, i_flags is operated in
bitwise context, but those assignments are only happened at the time of
initializing the corresponding OCFS2 private on-disk inode info.  Hence
there is no impact from the end-user's perspective.

However, it's better to replace those three assignments with bitwise
operations
since they essentially should be.  In this way, we can immediately know that the
business performed on i_flags are in bitwise rather than increasing or
decreasing
a count value. 
> For the usual reason: so I and others can decide which kernel versions
> need the fix.
The current fix comments looks too horrible than it would be.
Can it make your life easier if I repost this patch with the revised
descriptions if Joseph agrees to me?


Thanks,
-Jeff