* [patch] xfs: bug widening binary "not" operation
@ 2013-05-16 7:53 Dan Carpenter
2013-05-16 23:03 ` Dave Chinner
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2013-05-16 7:53 UTC (permalink / raw)
To: Ben Myers; +Cc: Alex Elder, kernel-janitors, xfs
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.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 1501f4f..9f557c6 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -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;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [patch] xfs: bug widening binary "not" operation
2013-05-16 7:53 [patch] xfs: bug widening binary "not" operation Dan Carpenter
@ 2013-05-16 23:03 ` Dave Chinner
2013-05-17 6:31 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Dave Chinner @ 2013-05-16 23:03 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ben Myers, Alex Elder, kernel-janitors, xfs
On Thu, May 16, 2013 at 10:53:30AM +0300, Dan Carpenter wrote:
> 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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] xfs: bug widening binary "not" operation
2013-05-16 23:03 ` Dave Chinner
@ 2013-05-17 6:31 ` Dan Carpenter
2013-05-17 10:19 ` Dave Chinner
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2013-05-17 6:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: Ben Myers, Alex Elder, kernel-janitors, xfs
On Fri, May 17, 2013 at 09:03:14AM +1000, Dave Chinner wrote:
> 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
>
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
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] xfs: bug widening binary "not" operation
2013-05-17 6:31 ` Dan Carpenter
@ 2013-05-17 10:19 ` Dave Chinner
0 siblings, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2013-05-17 10:19 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ben Myers, Alex Elder, kernel-janitors, xfs
On Fri, May 17, 2013 at 09:31:59AM +0300, Dan Carpenter wrote:
> On Fri, May 17, 2013 at 09:03:14AM +1000, Dave Chinner wrote:
> > 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
> >
>
> 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).
>
> - 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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-17 10:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-16 7:53 [patch] xfs: bug widening binary "not" operation Dan Carpenter
2013-05-16 23:03 ` Dave Chinner
2013-05-17 6:31 ` Dan Carpenter
2013-05-17 10:19 ` Dave Chinner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox