On Fri, Jul 08, 2016 at 09:14:55AM +1000, Dave Chinner wrote:
> On Thu, Jul 07, 2016 at 12:18:13PM -0700, Darrick J. Wong wrote:
> > On Thu, Jul 07, 2016 at 02:41:56PM -0400, Brian Foster wrote:
> > > > + if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
> > > > + return false;
> > > > + if (!xfs_btree_sblock_v5hdr_verify(bp))
> > > > + return false;
> > > > +
> > > > + level = be16_to_cpu(block->bb_level);
> > > > + if (pag && pag->pagf_init) {
> > > > + if (level >= pag->pagf_levels[XFS_BTNUM_RMAPi])
> > > > + return false;
> > > > + } else if (level >= mp->m_rmap_maxlevels)
> > > > + return false;
> > >
> > > It looks like the above (level >= mp->m_rmap_maxlevels) check could be
> > > independent (rather than an 'else). Otherwise looks good:
> >
> > Hmmm.... at first I wondered, "Shouldn't we have already checked that
> > pag->pagf_levels[XFS_BTNUM_RMAPi] <= mp->m_rmap_maxlevels?" But then I
> > realized that no, we don't do that anywhere. Nor does the bnobt/cntbt
> > verifier. Am I missing something?
(Yes, I am.)
> It should have been ranged checked when the AGF is first read in
> (i.e. in the verifier), in ASSERTS every time xfs_alloc_read_agf()
> is called after initialisation, and then every time the verifier is
> run on write of the AGF.
You're right. I missed that. :(
> > I did see that we at least check the AGF/AGI levels to make sure they don't
> > overflow XFS_BTREE_MAXLEVELS, so we're probably fine here.
>
> Precisely - if the AGF verifier doesn't have a max level check in it
> for the rmapbt, then we need to add one there.
There's a check there, so we're fine.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
|