* [PATCH] xfs: fix error handling in xfs_bmap_extents_to_btree
@ 2018-09-30 6:18 Dave Chinner
2018-09-30 15:34 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2018-09-30 6:18 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
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@redhat.com>
---
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) {
- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] xfs: fix error handling in xfs_bmap_extents_to_btree
2018-09-30 6:18 [PATCH] xfs: fix error handling in xfs_bmap_extents_to_btree Dave Chinner
@ 2018-09-30 15:34 ` Darrick J. Wong
2018-09-30 22:09 ` Dave Chinner
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2018-09-30 15:34 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Sun, Sep 30, 2018 at 04:18:07PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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@redhat.com>
> ---
> 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@oracle.com>
--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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] xfs: fix error handling in xfs_bmap_extents_to_btree
2018-09-30 15:34 ` Darrick J. Wong
@ 2018-09-30 22:09 ` Dave Chinner
0 siblings, 0 replies; 3+ messages in thread
From: Dave Chinner @ 2018-09-30 22:09 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Sun, Sep 30, 2018 at 08:34:44AM -0700, Darrick J. Wong wrote:
> On Sun, Sep 30, 2018 at 04:18:07PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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:
.....
> > @@ -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?
Not me. I had a brief thought about this when looking at this,
but I think we really need to start from the bottom up (i.e. from
the xfs_buf_* functions first) before moving to the higher level
code.
> (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@oracle.com>
Thanks Darrick, I'll add it to the fixes branch (it's been on top of
the merged tree I've been testing over the weekend) and get it out.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-01 4:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-30 6:18 [PATCH] xfs: fix error handling in xfs_bmap_extents_to_btree Dave Chinner
2018-09-30 15:34 ` Darrick J. Wong
2018-09-30 22:09 ` Dave Chinner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).