diff mbox series

[1/3] xfs: clean up bmap intent item recovery checking

Message ID 160031337029.3624582.3821482653073391016.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: fix inode use-after-free during log recovery | expand

Commit Message

Darrick J. Wong Sept. 17, 2020, 3:29 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

The bmap intent item checking code in xfs_bui_item_recover is spread all
over the function.  We should check the recovered log item at the top
before we allocate any resources or do anything else, so do that.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_item.c |   57 ++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

Comments

Dave Chinner Sept. 17, 2020, 5:09 a.m. UTC | #1 
On Wed, Sep 16, 2020 at 08:29:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The bmap intent item checking code in xfs_bui_item_recover is spread all
> over the function.  We should check the recovered log item at the top
> before we allocate any resources or do anything else, so do that.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_item.c |   57 ++++++++++++++++--------------------------------
>  1 file changed, 19 insertions(+), 38 deletions(-)

Looks good - I have a very similar patch here....

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Christoph Hellwig Sept. 23, 2020, 7:16 a.m. UTC | #2 
On Wed, Sep 16, 2020 at 08:29:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The bmap intent item checking code in xfs_bui_item_recover is spread all
> over the function.  We should check the recovered log item at the top
> before we allocate any resources or do anything else, so do that.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_item.c |   57 ++++++++++++++++--------------------------------
>  1 file changed, 19 insertions(+), 38 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 0a0904a7650a..877afe76d76a 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -437,17 +437,13 @@ xfs_bui_item_recover(
>  	xfs_fsblock_t			inode_fsb;
>  	xfs_filblks_t			count;
>  	xfs_exntst_t			state;
> -	enum xfs_bmap_intent_type	type;
> -	bool				op_ok;
>  	unsigned int			bui_type;
>  	int				whichfork;
>  	int				error = 0;
>  
>  	/* Only one mapping operation per BUI... */
> -	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
> -		xfs_bui_release(buip);
> -		return -EFSCORRUPTED;
> -	}
> +	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS)
> +		goto garbage;

We don't really need the xfs_bui_release any more, and can stick to
plain "return -EFSCORRUPTED" instead of the goto, but I suspect the
previous patch has taken care of that and you've rebased already?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Sept. 23, 2020, 3:22 p.m. UTC | #3 
On Wed, Sep 23, 2020 at 08:16:14AM +0100, Christoph Hellwig wrote:
> On Wed, Sep 16, 2020 at 08:29:30PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The bmap intent item checking code in xfs_bui_item_recover is spread all
> > over the function.  We should check the recovered log item at the top
> > before we allocate any resources or do anything else, so do that.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_item.c |   57 ++++++++++++++++--------------------------------
> >  1 file changed, 19 insertions(+), 38 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> > index 0a0904a7650a..877afe76d76a 100644
> > --- a/fs/xfs/xfs_bmap_item.c
> > +++ b/fs/xfs/xfs_bmap_item.c
> > @@ -437,17 +437,13 @@ xfs_bui_item_recover(
> >  	xfs_fsblock_t			inode_fsb;
> >  	xfs_filblks_t			count;
> >  	xfs_exntst_t			state;
> > -	enum xfs_bmap_intent_type	type;
> > -	bool				op_ok;
> >  	unsigned int			bui_type;
> >  	int				whichfork;
> >  	int				error = 0;
> >  
> >  	/* Only one mapping operation per BUI... */
> > -	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
> > -		xfs_bui_release(buip);
> > -		return -EFSCORRUPTED;
> > -	}
> > +	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS)
> > +		goto garbage;
> 
> We don't really need the xfs_bui_release any more, and can stick to
> plain "return -EFSCORRUPTED" instead of the goto, but I suspect the
> previous patch has taken care of that and you've rebased already?

Yep.

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 0a0904a7650a..877afe76d76a 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -437,17 +437,13 @@  xfs_bui_item_recover(
 	xfs_fsblock_t			inode_fsb;
 	xfs_filblks_t			count;
 	xfs_exntst_t			state;
-	enum xfs_bmap_intent_type	type;
-	bool				op_ok;
 	unsigned int			bui_type;
 	int				whichfork;
 	int				error = 0;
 
 	/* Only one mapping operation per BUI... */
-	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS) {
-		xfs_bui_release(buip);
-		return -EFSCORRUPTED;
-	}
+	if (buip->bui_format.bui_nextents != XFS_BUI_MAX_FAST_EXTENTS)
+		goto garbage;
 
 	/*
 	 * First check the validity of the extent described by the
@@ -458,29 +454,26 @@  xfs_bui_item_recover(
 			   XFS_FSB_TO_DADDR(mp, bmap->me_startblock));
 	inode_fsb = XFS_BB_TO_FSB(mp, XFS_FSB_TO_DADDR(mp,
 			XFS_INO_TO_FSB(mp, bmap->me_owner)));
-	switch (bmap->me_flags & XFS_BMAP_EXTENT_TYPE_MASK) {
+	state = (bmap->me_flags & XFS_BMAP_EXTENT_UNWRITTEN) ?
+			XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
+	whichfork = (bmap->me_flags & XFS_BMAP_EXTENT_ATTR_FORK) ?
+			XFS_ATTR_FORK : XFS_DATA_FORK;
+	bui_type = bmap->me_flags & XFS_BMAP_EXTENT_TYPE_MASK;
+	switch (bui_type) {
 	case XFS_BMAP_MAP:
 	case XFS_BMAP_UNMAP:
-		op_ok = true;
 		break;
 	default:
-		op_ok = false;
-		break;
+		goto garbage;
 	}
-	if (!op_ok || startblock_fsb == 0 ||
+	if (startblock_fsb == 0 ||
 	    bmap->me_len == 0 ||
 	    inode_fsb == 0 ||
 	    startblock_fsb >= mp->m_sb.sb_dblocks ||
 	    bmap->me_len >= mp->m_sb.sb_agblocks ||
 	    inode_fsb >= mp->m_sb.sb_dblocks ||
-	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS)) {
-		/*
-		 * This will pull the BUI from the AIL and
-		 * free the memory associated with it.
-		 */
-		xfs_bui_release(buip);
-		return -EFSCORRUPTED;
-	}
+	    (bmap->me_flags & ~XFS_BMAP_EXTENT_FLAGS))
+		goto garbage;
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate,
 			XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK), 0, 0, &tp);
@@ -501,32 +494,17 @@  xfs_bui_item_recover(
 	if (VFS_I(ip)->i_nlink == 0)
 		xfs_iflags_set(ip, XFS_IRECOVERY);
 
-	/* Process deferred bmap item. */
-	state = (bmap->me_flags & XFS_BMAP_EXTENT_UNWRITTEN) ?
-			XFS_EXT_UNWRITTEN : XFS_EXT_NORM;
-	whichfork = (bmap->me_flags & XFS_BMAP_EXTENT_ATTR_FORK) ?
-			XFS_ATTR_FORK : XFS_DATA_FORK;
-	bui_type = bmap->me_flags & XFS_BMAP_EXTENT_TYPE_MASK;
-	switch (bui_type) {
-	case XFS_BMAP_MAP:
-	case XFS_BMAP_UNMAP:
-		type = bui_type;
-		break;
-	default:
-		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
-		error = -EFSCORRUPTED;
-		goto err_inode;
-	}
 	xfs_trans_ijoin(tp, ip, 0);
 
 	count = bmap->me_len;
-	error = xfs_trans_log_finish_bmap_update(tp, budp, type, ip, whichfork,
-			bmap->me_startoff, bmap->me_startblock, &count, state);
+	error = xfs_trans_log_finish_bmap_update(tp, budp, bui_type, ip,
+			whichfork, bmap->me_startoff, bmap->me_startblock,
+			&count, state);
 	if (error)
 		goto err_inode;
 
 	if (count > 0) {
-		ASSERT(type == XFS_BMAP_UNMAP);
+		ASSERT(bui_type == XFS_BMAP_UNMAP);
 		irec.br_startblock = bmap->me_startblock;
 		irec.br_blockcount = count;
 		irec.br_startoff = bmap->me_startoff;
@@ -546,6 +524,9 @@  xfs_bui_item_recover(
 		xfs_irele(ip);
 	}
 	return error;
+garbage:
+	xfs_bui_release(buip);
+	return -EFSCORRUPTED;
 }
 
 STATIC bool