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@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
