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?
> - 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.
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?
> + }
> + 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?
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
