On Sun, Sep 30, 2018 at 04:18:07PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Commit 01239d77b9dd ("xfs: fix a null pointer dereference in
> xfs_bmap_extents_to_btree") attempted to fix a null pointer
> dreference when a fuzzing corruption of some kind was found.
> This fix was flawed, resulting in assert failures like:
>
> XFS: Assertion failed: ifp->if_broot == NULL, file: fs/xfs/libxfs/xfs_bmap.c, line: 715
> .....
> Call Trace:
> xfs_bmap_extents_to_btree+0x6b9/0x7b0
> __xfs_bunmapi+0xae7/0xf00
> ? xfs_log_reserve+0x1c8/0x290
> xfs_reflink_remap_extent+0x20b/0x620
> xfs_reflink_remap_blocks+0x7e/0x290
> xfs_reflink_remap_range+0x311/0x530
> vfs_dedupe_file_range_one+0xd7/0xe0
> vfs_dedupe_file_range+0x15b/0x1a0
> do_vfs_ioctl+0x267/0x6c0
>
> The problem is that the error handling code now asserts that the
> inode fork is not in btree format before the error handling code
> undoes the modifications that put the fork back in extent format.
> Fix this by moving the assert back to after the xfs_iroot_realloc()
> call that returns the fork to extent format, and clean up the jump
> labels to be meaningful.
>
> Also, returning ENOSPC when xfs_btree_get_bufl() fails to
> instantiate the buffer that was allocated (the actual fix in the
> commit mentioned above) is incorrect. This is a fatal error - only
> an invalid block address or a filesystem shutdown can result in
> failing to get a buffer here.
>
> Hence change this to EFSCORRUPTED so that the higher layer knows
> this was a corruption related failure and should not treat it as an
> ENOSPC error. This should result in a shutdown (via cancelling a
> dirty transaction) which is necessary as we do not attempt to clean
> up the (invalid) block that we have already allocated.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 2760314fdf7f..a47670332326 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -673,7 +673,8 @@ xfs_bmap_extents_to_btree(
> ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS);
>
> /*
> - * Make space in the inode incore.
> + * Make space in the inode incore. This needs to be undone if we fail
> + * to expand the root.
> */
> xfs_iroot_realloc(ip, 1, whichfork);
> ifp->if_flags |= XFS_IFBROOT;
> @@ -711,16 +712,15 @@ xfs_bmap_extents_to_btree(
> args.minlen = args.maxlen = args.prod = 1;
> args.wasdel = wasdel;
> *logflagsp = 0;
> - if ((error = xfs_alloc_vextent(&args))) {
> - ASSERT(ifp->if_broot == NULL);
> - goto err1;
> - }
> + error = xfs_alloc_vextent(&args);
> + if (error)
> + goto out_root_realloc;
>
> if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) {
> - ASSERT(ifp->if_broot == NULL);
> error = -ENOSPC;
> - goto err1;
> + goto out_root_realloc;
> }
> +
> /*
> * Allocation can't fail, the space was reserved.
> */
> @@ -732,9 +732,10 @@ xfs_bmap_extents_to_btree(
> xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L);
> abp = xfs_btree_get_bufl(mp, tp, args.fsbno, 0);
> if (!abp) {
This reminds me to ask, has anyone made progress converting the
{get,read}_buf functions (and associated callers) to return error codes?
(I wasn't expecting that as a part of this fix; I'm simply recollecting
my marbles.)
Anyway, this looks ok to me.
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> - error = -ENOSPC;
> - goto err2;
> + error = -EFSCORRUPTED;
> + goto out_unreserve_dquot;
> }
> +
> /*
> * Fill in the child block.
> */
> @@ -775,11 +776,12 @@ xfs_bmap_extents_to_btree(
> *logflagsp = XFS_ILOG_CORE | xfs_ilog_fbroot(whichfork);
> return 0;
>
> -err2:
> +out_unreserve_dquot:
> xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
> -err1:
> +out_root_realloc:
> xfs_iroot_realloc(ip, -1, whichfork);
> XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> + ASSERT(ifp->if_broot == NULL);
> xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
>
> return error;
> --
> 2.17.0
>
