Re: [PATCH 06/11] xfs_repair: check v5 filesystem attr block header sanity

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 25, 2015 at 05:59:11PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 26, 2015 at 10:45:02AM +1000, Dave Chinner wrote:
> > On Tue, Aug 25, 2015 at 05:32:59PM -0700, Darrick J. Wong wrote:
> > > Check the v5 fields (uuid, blocknr, owner) of attribute blocks for
> > > obvious errors while scanning xattr blocks.  If the ownership info
> > > is incorrect, kill the block.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Why hasn't the buffer verifier done this validation?
> 
> Maybe I'm confused here, so here's what I think is going on:
> 
> AFAICT most of the verifiers do things like this:
> 
> if (crcs_enabled && cksum_verification fails) {
> 	xfs_buf_ioerror(bp, -EFSBADCRC);
> } else if (header_is_insane) {
> 	xfs_buf_ioerror(bp, -EFSCORRUPTED);
> }
> 
> The fuzzer corrupts the UUID without updating the CRC.  The verifier first
> checks the CRC and it doesn't match, so it sets b_error to -EFSBADCRC and
> doesn't get to the header check. 

Ok, that explains it - I didn't consider that case. This would seem
like a general problem for repair when CRC errors are detected? i.e.
we set the repair flag without doing the remaining verifier validity
checks?

As it is, I don't really like duplicating the verifier checks in
repair. ISTR I recently suggested that we need to factor all the
common verifier checks (magic, owner, uuid, blockno) into a single
function that all verifiers called to remove all the code
duplication. If we do this, then repair can also call the function
to verify headers after a CRC failure to determine if repair is
possible....

This is a bit more work, so I'll probably take this specific patch
for 4.2.0, but I'd like to see this all factored out so we aren't
duplicating code unnecessarily.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs