Re: [PATCH 3/7] xfs: filter out obviously bad btree pointers

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jan 30, 2017 at 10:39:05PM -0600, Eric Sandeen wrote:
> On 1/30/17 6:23 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Don't let anybody load an obviously bad btree pointer.  Since the values
> > come from disk, we must return an error, not just ASSERT.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c  |    4 +---
> >  fs/xfs/libxfs/xfs_btree.c |    3 ++-
> >  fs/xfs/libxfs/xfs_btree.h |    2 +-
> >  3 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index bfc00de..443cb10 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1291,9 +1291,7 @@ xfs_bmap_read_extents(
> >  	ASSERT(level > 0);
> >  	pp = XFS_BMAP_BROOT_PTR_ADDR(mp, block, 1, ifp->if_broot_bytes);
> >  	bno = be64_to_cpu(*pp);
> > -	ASSERT(bno != NULLFSBLOCK);
> > -	ASSERT(XFS_FSB_TO_AGNO(mp, bno) < mp->m_sb.sb_agcount);
> > -	ASSERT(XFS_FSB_TO_AGBNO(mp, bno) < mp->m_sb.sb_agblocks);
> > +
> 
> fwiw there's an assignment of bno = NULLFSBLOCK just before here,
> which is never used and overwritten in this hunk.
> Could probably clean that up here too.

Yes, that could go too.

> >  	/*
> >  	 * Go down the tree until leaf level is reached, following the first
> >  	 * pointer (leftmost) at each level.
> > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> > index 21e6a6a..2849d3f 100644
> > --- a/fs/xfs/libxfs/xfs_btree.c
> > +++ b/fs/xfs/libxfs/xfs_btree.c
> > @@ -810,7 +810,8 @@ xfs_btree_read_bufl(
> >  	xfs_daddr_t		d;		/* real disk block address */
> >  	int			error;
> >  
> > -	ASSERT(fsbno != NULLFSBLOCK);
> > +	if (!XFS_FSB_SANITY_CHECK(mp, fsbno))
> > +		return -EFSCORRUPTED;
> 
> This does away with the NULLFSBLOCK checks though, right?
> 
> #define NULLFSBLOCK     ((xfs_fsblock_t)-1)
> 
> which is also used as an in-memory condition, so I'm not sure
> it should be added to XFS_FSB_SANITY_CHECK.

[reiterating our irc conversation]

It shouldn't, since XFS_FSB_TO_AGNO(mp, NULLFSBLOCK) ought to produce
NULLAGNUMBER, which will still fail the check.

> IOWs some asserts about it are really code flow asserts, though
> it also shouldn't be read from disk.

xfs_btree_read_bufl is called by xfs_bmap_check_leaf_extents, which is
the verifier of the on-disk data.

--D

> >  	d = XFS_FSB_TO_DADDR(mp, fsbno);
> >  	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp, d,
> >  				   mp->m_bsize, lock, &bp, ops);
> > diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> > index b69b947..33a8f86 100644
> > --- a/fs/xfs/libxfs/xfs_btree.h
> > +++ b/fs/xfs/libxfs/xfs_btree.h
> > @@ -456,7 +456,7 @@ static inline int xfs_btree_get_level(struct xfs_btree_block *block)
> >  #define	XFS_FILBLKS_MAX(a,b)	max_t(xfs_filblks_t, (a), (b))
> >  
> >  #define	XFS_FSB_SANITY_CHECK(mp,fsb)	\
> > -	(XFS_FSB_TO_AGNO(mp, fsb) < mp->m_sb.sb_agcount && \
> > +	(fsb && XFS_FSB_TO_AGNO(mp, fsb) < mp->m_sb.sb_agcount && \
> >  		XFS_FSB_TO_AGBNO(mp, fsb) < mp->m_sb.sb_agblocks)
> >  
> >  /*
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html