xfs: bug widening binary "not" operation
Subsystems (2)
PATCH 1/1 xfs: bug widening binary "not" operation
The problem here is:
ioffset = offset & ~(rounding - 1);
"offset" and "ioffset" are type xfs_off_t (__s64) and "rounding" is unsigned int. The "offset & ~(rounding - 1)" clears the high 32 bits and which is unintentional.
This is a static checker fix so I'm not sure how much difference this makes in real life.
fs/xfs/xfs_vnodeops.c +1 -1
@@ -1453,7 +1453,7 @@ xfs_free_file_space(xfs_mount_t *mp;int nimap;uint resblks; -uint rounding; +xfs_off_t rounding;int rt;xfs_fileoff_t startoffset_fsb;xfs_trans_t *tp;
Quoted 10 lines · Dan Carpenter
The problem here is:
ioffset = offset & ~(rounding - 1);"offset" and "ioffset" are type
xfs_off_t(__s64) and "rounding" is unsigned int. The "offset & ~(rounding - 1)" clears the high 32 bits and which is unintentional.This is a static checker fix so I'm not sure how much difference this makes in real life.
It is a real problem, but one that is masked by the way we do range flushing right now.
As it is, the static checker missed the:
rounding = max_t(uint, ....);
The line before the above usage. I posted a patch to fix this this 2 weeks ago here:
http://oss.sgi.com/pipermail/xfs/2013-May/025986.html
But thanks for the independent confirmation of the problem, Dan. ;)
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 9 lines · Dave Chinner
As it is, the static checker missed the:
rounding = max_t(uint, ....);The line before the above usage. I posted a patch to fix this this 2 weeks ago here:
Ah. Grand. There is still a problem with the max_t(). The shift operation will wrap before we do the cast. It should be:
- rounding = max_t(xfs_off_t, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
+ rounding = max_t(xfs_off_t, 1ULL << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
regards,
dan carpenter
Quoted 5 lines · Dan Carpenter
Quoted 4 lines · Dave Chinner
As it is, the static checker missed the:
rounding = max_t(uint, ....);
Quoted 8 lines
Quoted 5 lines
The line before the above usage. I posted a patch to fix this this 2 weeks ago here:
Ah. Grand. There is still a problem with the
max_t(). The shift operation will wrap before we do the cast. It should be:
It probably looks that way, but it can't overflow as mp->m_sb.sb_blocklog has a maximum value of 16 (i.e. 64k maximum filesystem block size).
Review
Quoted 3 lines
(quoted diff) +1
-1- rounding = max_t(xfs_off_t, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE); + rounding = max_t(xfs_off_t, 1ULL << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
So it is safe the way it is and this is not necessary.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]