On Thu, Aug 11, 2011 at 03:33:38PM -0600, Andreas Dilger wrote:
> On 2011-08-11, at 3:13 PM, Darrick J. Wong wrote:
> > It turns out that ext4_ext_check only verifies the validity of the extent block
> > it's processing if the block has to be read in from the disk. Unfortunately,
> > this means that the check is NOT done if the block is already in memory, which
> > means that if a file has a corrupted extent block, then the first IO peformed
> > on the file will find the corrupt block and fail, but a second IO will see that
> > the extent block is in memory, bypass the corruption check, and use garbage
> > data as if they were extent data.
>
> It looks like ext4_ext_check() is fairly heavyweight, so calling it on every
> extent access may affect performance. What about marking the extent or buffer
<shrug> I didn't think the simple header check was too terribly heavy...
... but it'll get more heavyweight when you add in metadata checksumming. :)
> bad in some way so that it always gets checked? In the ext2 directory code
> it marks a directory page with PG_checked to indicate that it was validated
> on read, but there could be a number of different mechanisms to do this
> (including setting a bit in the magic so that ext4_ext_check() is aborted
> very quickly, possibly without any additional error on the console since
> one would already have been printed).
Ok, I guess I could add a BH_Checked = BH_JBDPrivateStart flag to ext4 and use
that to bypass the header check (and especially the checksum check) if it's
set. Yes, I like that idea more... :)
Come to think of it I could probably reuse this in other places like the
directory handling code. Okay, I'll roll that in.
--D
>
> > A simple testcase is to allocate a file with enough extents to overflow the
> > inode i_block, umount, overwrite the extent block magic with garbage, then
> > mount the filesystem and try to access the file. The first access causes the
> > kernel to spit out an error, but subsequent accesses seem to succeed.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> >
> > fs/ext4/extents.c | 6 +-----
> > 1 files changed, 1 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index ee4b391..bb07b79 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -744,8 +744,6 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
> > i = depth;
> > /* walk through the tree */
> > while (i) {
> > - int need_to_validate = 0;
> > -
> > ext_debug("depth %d: num %d, max %d\n",
> > ppos, le16_to_cpu(eh->eh_entries), le16_to_cpu(eh->eh_max));
> >
> > @@ -764,8 +762,6 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
> > put_bh(bh);
> > goto err;
> > }
> > - /* validate the extent entries */
> > - need_to_validate = 1;
> > }
> > eh = ext_block_hdr(bh);
> > ppos++;
> > @@ -779,7 +775,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
> > path[ppos].p_hdr = eh;
> > i--;
> >
> > - if (need_to_validate && ext4_ext_check(inode, eh, i))
> > + if (ext4_ext_check(inode, eh, i))
> > goto err;
> > }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> Cheers, Andreas
>
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
