Re: [PATCH 04/10] xfs: check directory bestfree information in the verifier

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jun 04, 2018 at 10:10:19AM +1000, Dave Chinner wrote:
> On Sun, Jun 03, 2018 at 04:22:49PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Create a variant of xfs_dir2_data_freefind that is suitable for use in a
> > verifier.  Because _freefind is called by the verifier, we simply
> > duplicate the _freefind function, convert the ASSERTs to return
> > __this_address, and modify the verifier to call our new function.  Once
> > we've made it impossible for directory blocks with bad bestfree data to
> > make it into the filesystem we can remove the DEBUG code from the
> > regular _freefind function.
> > 
> > Underlying argument: corruption of on-disk metadata should return
> > -EFSCORRUPTED instead of blowing ASSERTs.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_dir2_data.c |  121 +++++++++++++++++++++++++++++------------
> >  1 file changed, 86 insertions(+), 35 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> > index cb67ec730b9b..bc5c0ba46ec6 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_data.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> > @@ -33,6 +33,11 @@
> >  #include "xfs_cksum.h"
> >  #include "xfs_log.h"
> >  
> > +static xfs_failaddr_t xfs_dir2_data_freefind_verify(
> > +		struct xfs_dir2_data_hdr *hdr, struct xfs_dir2_data_free *bf,
> > +		struct xfs_dir2_data_unused *dup,
> > +		struct xfs_dir2_data_free **bf_ent);
> > +
> >  /*
> >   * Check the consistency of the data block.
> >   * The input can also be a block-format directory.
> > @@ -52,6 +57,7 @@ __xfs_dir3_data_check(
> >  	xfs_dir2_data_free_t	*dfp;		/* bestfree entry */
> >  	xfs_dir2_data_unused_t	*dup;		/* unused entry */
> >  	char			*endp;		/* end of useful data */
> > +	xfs_failaddr_t		fa;
> >  	int			freeseen;	/* mask of bestfrees seen */
> >  	xfs_dahash_t		hash;		/* hash of current name */
> >  	int			i;		/* leaf index */
> 
> This could be placed inside the loop scope, right?

Right.

> > -	ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
> > -	       hdr->magic == cpu_to_be32(XFS_DIR3_DATA_MAGIC) ||
> > -	       hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC) ||
> > -	       hdr->magic == cpu_to_be32(XFS_DIR3_BLOCK_MAGIC));
> > -	for (dfp = &bf[0], seenzero = matched = 0;
> > -	     dfp < &bf[XFS_DIR2_DATA_FD_COUNT];
> > -	     dfp++) {
> > +	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
> >  		if (!dfp->offset) {
> > -			ASSERT(!dfp->length);
> > -			seenzero = 1;
> > +			if (dfp->length)
> > +				return __this_address;
> > +			seenzero = true;
> >  			continue;
> >  		}
> > -		ASSERT(seenzero == 0);
> > +		if (seenzero)
> > +			return __this_address;
> >  		if (be16_to_cpu(dfp->offset) == off) {
> > -			matched = 1;
> > -			ASSERT(dfp->length == dup->length);
> > -		} else if (off < be16_to_cpu(dfp->offset))
> > -			ASSERT(off + be16_to_cpu(dup->length) <= be16_to_cpu(dfp->offset));
> > -		else
> > -			ASSERT(be16_to_cpu(dfp->offset) + be16_to_cpu(dfp->length) <= off);
> > -		ASSERT(matched || be16_to_cpu(dfp->length) >= be16_to_cpu(dup->length));
> > -		if (dfp > &bf[0])
> > -			ASSERT(be16_to_cpu(dfp[-1].length) >= be16_to_cpu(dfp[0].length));
> > +			matched = true;
> > +			if (dfp->length != dup->length)
> > +				return __this_address;
> > +		} else if (be16_to_cpu(dfp->offset) > off) {
> > +			if (off + be16_to_cpu(dup->length) >
> > +			    be16_to_cpu(dfp->offset))
> 
> can you indent the second line further to indicate it is a
> continuation of the logic statement on the previous line rather than
> a new logic condition? i.e.

Ok.

> 			if (off + be16_to_cpu(dup->length) >
> 					be16_to_cpu(dfp->offset))
> > +				return __this_address;
> > +		} else {
> > +			if (be16_to_cpu(dfp->offset) +
> > +			    be16_to_cpu(dfp->length) > off)
> > +				return __this_address;
> 
> Same here?

Fixed.

> > +		}
> > +		if (!matched &&
> > +		    be16_to_cpu(dfp->length) < be16_to_cpu(dup->length))
> > +			return __this_address;
> > +		if (dfp > &bf[0] &&
> > +		    be16_to_cpu(dfp[-1].length) < be16_to_cpu(dfp[0].length))
> > +			return __this_address;
> >  	}
> > -#endif
> > +
> > +	/*
> > +	 * If this is smaller than the smallest bestfree entry,
> > +	 * it can't be there since they're sorted.
> > +	 */
> > +	if (be16_to_cpu(dup->length) <
> > +	    be16_to_cpu(bf[XFS_DIR2_DATA_FD_COUNT - 1].length))
> > +		return NULL;
> > +	/*
> > +	 * Look at the three bestfree entries for our guy.
> > +	 */
> > +	for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
> > +		if (!dfp->offset)
> > +			return NULL;
> > +		if (be16_to_cpu(dfp->offset) == off) {
> > +			*bf_ent = dfp;
> > +			return NULL;
> > +		}
> > +	}
> > +	/*
> > +	 * Didn't find it.  This only happens if there are duplicate lengths.
> > +	 */
> > +	return NULL;
> 
> And this tail is basically a duplicate of what now remains in
> xfs_dir2_data_freefind(). Can you call that function rather than
> duplicating the search code?

Will do.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html