On Thu, Oct 14, 2021 at 10:10:36AM -0400, Brian Foster wrote:
> 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..?
Those fixes tags seem like a reasonable breadcrumb for finding fixes.
I'll add them to the respective patches on commit. So for this third
one:
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > 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?
Oh, I was idly wondering if these iterators ought to have one more
training wheel where the loop would be skipped entirely if you did
something buggy such as:
agno = mp->m_sb.sb_agcount;
/* time goes by */
for_each_perag_from(mp, agno...)
/* stuff */
Normally that would be skipped since xfs_perag_get(sb_agcount) returns
NULL, except in the case that it's racing with growfs. But, some
malfunction like this should be fairly easy to spot even in the common
case.
--D
> > Welcome back, by the way. :)
> >
>
> Thanks!
>
> Brian
>
> > --D
> >
> > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > > 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
> > >
> >
>
