Re: [PATCH] sector_t overflow in block layer
From: Andreas Dilger
Date: Thu May 18 2006 - 18:42:48 EST
On May 18, 2006 23:12 +0100, Anton Altaparmakov wrote:
> On Thu, 18 May 2006, Linus Torvalds wrote:
> > On Thu, 18 May 2006, Andreas Dilger wrote:
> > > + /* Check if we overflow sector_t when computing the sector offset. */
> > > + sector = (unsigned long long)bh->b_blocknr * (bh->b_size >> 9);
> > Ok so far, looks fine.
> > But what the heck is this:
> > > +#if !defined(CONFIG_LBD) && BITS_PER_LONG == 32
> > > + if (unlikely(sector != (sector_t)sector))
> > > +#else
> > > + if (unlikely(((bh->b_blocknr >> 32) * (bh->b_size >> 9)) >=
> > > + 0xffffffff00000000ULL))
> > > +#endif
> > I don't understand the #ifdef at all.
> > Why isn't that just a
> > if (unlikely(sector != (sector_t)sector))
> > and that's it? What does this have to do with CONFIG_LBD or BITS_PER_LONG,
> > or anything at all?
> > If the sector number fits in a sector_t, we're all good.
> I think you missed that Andreas said he is worried about 64-bit overflows
> as well. And you would not catch those with the sector !=
> (sector_t)sector test because you would be comparing two 64-bit values
> together so they always match...
> Hence why he shifts the value right by 32 bits then multiplies and tests
> the result for overflowing 32-bits which if it does it means it would
> overflow the 64-bit multiplication, too therefor your "sector" is
Yes, this is exactly correct. At various times while writing this I had
more comments to explain everything, but then removed them as too wordy.
The actual implementation isn't what I care about - the resulting corruption
is more important and I thought it better to at least get some attention
on it instead of worrying over the details.
Principal Software Engineer
Cluster File Systems, Inc.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/