On Thu, May 18, 2006 at 12:59:56PM -0600, Andreas Dilger wrote:
> At least the submit_bh() code should return an error in this case
> regardless of who the caller is. If ext[23] and other filesystems
> check this themselves at mount time that is of course also prudent.
>
> Patch has not been more than compile tested but it is a pretty serious
> problem dating back a long time so I'm posting it for review anyways.
>
> There are several ways to check for the 64-bit overflow, the efficiency
> of which depends on the architecture. We could alternately shift b_blocknr
> by >> 41 or change the mask, for example. The primary concern is really
> the 32-bit overflow and resulting silent and massive data corruption
> when CONFIG_LBD isn't enabled.
>
> It is coincidental that mke2fs will clobber the primary group's metadata
> (bitmaps, inode table) when formatting such a filesystem, but since this
> data is identical at format time (group 0 offsets and group 16384 offsets
> modulo 2TB are the same) it is very hard to detect before corruption hits.
>
> ==================== buffer_overflow.diff ===============================
> --- ./fs/buffer.c.orig 2006-05-18 12:15:45.000000000 -0600
> +++ ./fs/buffer.c 2006-05-18 12:09:20.000000000 -0600
> @@ -2758,12 +2784,32 @@ static int end_bio_bh_io_sync(struct bio
> int submit_bh(int rw, struct buffer_head * bh)
> {
> struct bio *bio;
> + unsigned long long sector;
> int ret = 0;
>
> BUG_ON(!buffer_locked(bh));
> BUG_ON(!buffer_mapped(bh));
> BUG_ON(!bh->b_end_io);
>
> + /* Check if we overflow sector_t when computing the sector offset. */
> + sector = (unsigned long long)bh->b_blocknr * (bh->b_size >> 9);
> +#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
> + {
> + printk(KERN_ERR "IO past maximum addressable sector"
> +#if !defined(CONFIG_LBD) && BITS_PER_LONG == 32
> + "- CONFIG_LBD not enabled"
> +#endif
> + "\n");
You may disagree, but this is ugly and you're touching generic code.
Why not hide it inside static inline function? And don't cut it into
pieces while you're at it.
#if !defined(CONFIG_LBD) && BITS_PER_LONG == 32
static inline
{
}
#else
static inline
{
}
#endif
> + buffer_io_error(bh);
> +
> + return -EOVERFLOW;
> + }
> +
> if (buffer_ordered(bh) && (rw == WRITE))
> rw = WRITE_BARRIER;
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
