* [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t @ 2013-09-04 2:41 Jeff Liu 2013-09-12 14:24 ` Ben Myers 0 siblings, 1 reply; 3+ messages in thread From: Jeff Liu @ 2013-09-04 2:41 UTC (permalink / raw) To: xfs@oss.sgi.com 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t 2013-09-04 2:41 [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t Jeff Liu @ 2013-09-12 14:24 ` Ben Myers 2013-09-13 10:06 ` Jeff Liu 0 siblings, 1 reply; 3+ messages in thread From: Ben Myers @ 2013-09-12 14:24 UTC (permalink / raw) To: Jeff Liu; +Cc: xfs@oss.sgi.com 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t 2013-09-12 14:24 ` Ben Myers @ 2013-09-13 10:06 ` Jeff Liu 0 siblings, 0 replies; 3+ messages in thread From: Jeff Liu @ 2013-09-13 10:06 UTC (permalink / raw) To: Ben Myers; +Cc: xfs@oss.sgi.com 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-09-13 10:05 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-04 2:41 [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t Jeff Liu 2013-09-12 14:24 ` Ben Myers 2013-09-13 10:06 ` Jeff Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox