Messages in this thread | | | Date | Fri, 19 May 2006 00:58:16 +0400 | From | Alexey Dobriyan <> | Subject | Re: [PATCH] sector_t overflow in block layer |
| |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |