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 >