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 |
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>
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>
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 --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