Re: [PATCH v2 3/4] xfs: terminate perag iteration reliably on agcount

[Brian Foster <bfoster@redhat.com>]

On Tue, Oct 12, 2021 at 12:08:22PM -0700, Darrick J. Wong wrote:
> On Tue, Oct 12, 2021 at 12:52:02PM -0400, Brian Foster wrote:
> > The for_each_perag_from() iteration macro relies on sb_agcount to
> > process every perag currently within EOFS from a given starting
> > point. It's perfectly valid to have perag structures beyond
> > sb_agcount, however, such as if a growfs is in progress. If a perag
> > loop happens to race with growfs in this manner, it will actually
> > attempt to process the post-EOFS perag where ->pag_agno ==
> > sb_agcount. This is reproduced by xfs/104 and manifests as the
> > following assert failure in superblock write verifier context:
> > 
> >  XFS: Assertion failed: agno < mp->m_sb.sb_agcount, file: fs/xfs/libxfs/xfs_types.c, line: 22
> > 
> > Update the corresponding macro to only process perags that are
> > within the current sb_agcount.
> 
> Does this need a Fixes: tag?
> 

Probably. I briefly looked into this originally, saw that this code was
introduced/modified across a span of commits and skipped it because it
wasn't immediately clear which singular commit may have introduced the
bug(s). Since these are now separate patches, I'd probably go with
58d43a7e3263 ("xfs: pass perags around in fsmap data dev functions") for
this one (since it introduced the use of sb_agcount) and f250eedcf762
("xfs: make for_each_perag... a first class citizen") for the next
patch.

That said, technically we could probably refer to the latter for both of
these fixes as a suitable enough catchall for the intended purpose of
the Fixes tag. I suspect the fundamental problem actually exists in that
base patch because for_each_perag() iterates solely based on pag !=
NULL. It seems a little odd that the sb_agcount usage is not introduced
until a couple patches later, but I suppose that could just be
considered a dependency. In reality, it's probably unlikely to ever have
a stable kernel at that intermediate point of a rework series so it
might not matter much either way. I don't really have a preference one
way or the other. Your call..?

> Also ... should we be checking for agno <= agcount-1 for the initial
> xfs_perag_get in the first for loop clause of for_each_perag_range?
> I /think/ the answer is that the current users are careful enough to
> check that race, but I haven't looked exhaustively.
> 

Not sure I follow... for_each_perag_range() is a more generic variant
that doesn't know or care about sb_agcount. I think it should support
the ability to span an arbitrary range of perags regardless of
sb_agcount. Hm?

> Welcome back, by the way. :)
> 

Thanks!

Brian

> --D
> 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_ag.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> > index cf8baae2ba18..b8cc5017efba 100644
> > --- a/fs/xfs/libxfs/xfs_ag.h
> > +++ b/fs/xfs/libxfs/xfs_ag.h
> > @@ -142,7 +142,7 @@ struct xfs_perag *xfs_perag_next(
> >  		(pag) = xfs_perag_next((pag), &(agno)))
> >  
> >  #define for_each_perag_from(mp, agno, pag) \
> > -	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount, (pag))
> > +	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1, (pag))
> >  
> >  
> >  #define for_each_perag(mp, agno, pag) \
> > -- 
> > 2.31.1
> > 
>