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:

quoted

On Tue, Oct 12, 2021 at 12:52:02PM -0400, Brian Foster wrote:

quoted

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@kernel.org>

quoted

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

quoted

Welcome back, by the way. :)

Thanks!

Brian

quoted

--D

quoted

Signed-off-by: Brian Foster <redacted>
---
 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