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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
