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?
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.
> 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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|