[PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t

[PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

Make the incore inode di_size to unsigned, this would be helpful
to catch the negative sizes of it in many cases, so that we don't
need to perform additional check for it being less than ZERO or not.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 fs/xfs/xfs_inode_fork.c |    3 +--
 fs/xfs/xfs_log_format.h |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
index 02f1083..2b60a5a 100644
--- a/fs/xfs/xfs_inode_fork.c
+++ b/fs/xfs/xfs_inode_fork.c
@@ -167,8 +167,7 @@ xfs_iformat_fork(
 			}
 
 			di_size = be64_to_cpu(dip->di_size);
-			if (unlikely(di_size < 0 ||
-				     di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
+			if (unlikely(di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
 				xfs_warn(ip->i_mount,
 			"corrupt inode %Lu (bad size %Ld for local inode).",
 					(unsigned long long) ip->i_ino,
diff --git a/fs/xfs/xfs_log_format.h b/fs/xfs/xfs_log_format.h
index a49ab2c..2795fc5 100644
--- a/fs/xfs/xfs_log_format.h
+++ b/fs/xfs/xfs_log_format.h
@@ -547,7 +547,7 @@ typedef struct xfs_icdinode {
 	xfs_ictimestamp_t di_atime;	/* time last accessed */
 	xfs_ictimestamp_t di_mtime;	/* time last modified */
 	xfs_ictimestamp_t di_ctime;	/* time created/inode modified */
-	xfs_fsize_t	di_size;	/* number of bytes in file */
+	xfs_ufsize_t	di_size;	/* number of bytes in file */
 	xfs_drfsbno_t	di_nblocks;	/* # of direct & btree blocks used */
 	xfs_extlen_t	di_extsize;	/* basic/minimum extent size for file */
 	xfs_extnum_t	di_nextents;	/* number of extents in data fork */
-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t

[Ben Myers]

Hey Jeff,

On Wed, Sep 04, 2013 at 10:41:56AM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> Make the incore inode di_size to unsigned, this would be helpful
> to catch the negative sizes of it in many cases, so that we don't
> need to perform additional check for it being less than ZERO or not.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> ---
>  fs/xfs/xfs_inode_fork.c |    3 +--
>  fs/xfs/xfs_log_format.h |    2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
> index 02f1083..2b60a5a 100644
> --- a/fs/xfs/xfs_inode_fork.c
> +++ b/fs/xfs/xfs_inode_fork.c
> @@ -167,8 +167,7 @@ xfs_iformat_fork(
>  			}
>  
>  			di_size = be64_to_cpu(dip->di_size);
> -			if (unlikely(di_size < 0 ||
> -				     di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
> +			if (unlikely(di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
>  				xfs_warn(ip->i_mount,
>  			"corrupt inode %Lu (bad size %Ld for local inode).",
>  					(unsigned long long) ip->i_ino,
> diff --git a/fs/xfs/xfs_log_format.h b/fs/xfs/xfs_log_format.h
> index a49ab2c..2795fc5 100644
> --- a/fs/xfs/xfs_log_format.h
> +++ b/fs/xfs/xfs_log_format.h
> @@ -547,7 +547,7 @@ typedef struct xfs_icdinode {
>  	xfs_ictimestamp_t di_atime;	/* time last accessed */
>  	xfs_ictimestamp_t di_mtime;	/* time last modified */
>  	xfs_ictimestamp_t di_ctime;	/* time created/inode modified */
> -	xfs_fsize_t	di_size;	/* number of bytes in file */
> +	xfs_ufsize_t	di_size;	/* number of bytes in file */

These two changes by themselves look fairly innocuous, but upon closer
inspection I'm not so sure...

e.g. xfs_fsize_t is still signed, and i_size is loff_t is still signed.
I'm wondering if this doesn't represent a subtle change in the on-disk
format for inodes up in that size range.  This was on my 3.12 queue.  I
think it bears more discussion, so I'll hold off on this one for now.

FWIW I believe we're still ok with just Dan's fix because the maximum
size for local format is half a block or less.

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t

[Jeff Liu]

Hi Ben,

On 09/12/2013 10:24 PM, Ben Myers wrote:

> Hey Jeff,
> 
> On Wed, Sep 04, 2013 at 10:41:56AM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> Make the incore inode di_size to unsigned, this would be helpful
>> to catch the negative sizes of it in many cases, so that we don't
>> need to perform additional check for it being less than ZERO or not.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>> ---
>>  fs/xfs/xfs_inode_fork.c |    3 +--
>>  fs/xfs/xfs_log_format.h |    2 +-
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
>> index 02f1083..2b60a5a 100644
>> --- a/fs/xfs/xfs_inode_fork.c
>> +++ b/fs/xfs/xfs_inode_fork.c
>> @@ -167,8 +167,7 @@ xfs_iformat_fork(
>>  			}
>>  
>>  			di_size = be64_to_cpu(dip->di_size);
>> -			if (unlikely(di_size < 0 ||
>> -				     di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
>> +			if (unlikely(di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
>>  				xfs_warn(ip->i_mount,
>>  			"corrupt inode %Lu (bad size %Ld for local inode).",
>>  					(unsigned long long) ip->i_ino,
>> diff --git a/fs/xfs/xfs_log_format.h b/fs/xfs/xfs_log_format.h
>> index a49ab2c..2795fc5 100644
>> --- a/fs/xfs/xfs_log_format.h
>> +++ b/fs/xfs/xfs_log_format.h
>> @@ -547,7 +547,7 @@ typedef struct xfs_icdinode {
>>  	xfs_ictimestamp_t di_atime;	/* time last accessed */
>>  	xfs_ictimestamp_t di_mtime;	/* time last modified */
>>  	xfs_ictimestamp_t di_ctime;	/* time created/inode modified */
>> -	xfs_fsize_t	di_size;	/* number of bytes in file */
>> +	xfs_ufsize_t	di_size;	/* number of bytes in file */
> 
> These two changes by themselves look fairly innocuous, but upon closer
> inspection I'm not so sure...

> 
> e.g. xfs_fsize_t is still signed, and i_size is loff_t is still signed.
> I'm wondering if this doesn't represent a subtle change in the on-disk
> format for inodes up in that size range.  This was on my 3.12 queue.  I
> think it bears more discussion, so I'll hold off on this one for now.

As per my understanding for Dave's comments, maybe it's better to make
di_size to be unsigned with few changes, that's why I chose to fix the
incore di_size only.  Hence if a negative di_size is read from the disk
to the incore in above case with Dan's fix, it will be evaluated to be
an unexpected large value which would cause the corresponding check up fails.

Similarly, if a negative value is assigned to the incore di_size, it'll be
evaluated to an unexpected bigger value as well. In consequence, after flushing
the incore structure to disk, and read it back again, it will go through
the previous check up again. 

> 
> FWIW I believe we're still ok with just Dan's fix because the maximum
> size for local format is half a block or less.

Ok, meanwhile, let's waiting for other's comments.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs