[PATCH v2 0/4] xfs: fix perag iteration raciness

[PATCH v2 0/4] xfs: fix perag iteration raciness

[Brian Foster]

v2:
- Factoring and patch granularity.
v1: https://lore.kernel.org/linux-xfs/20211007125053.1096868-1-bfoster@redhat.com/

Brian Foster (4):
  xfs: fold perag loop iteration logic into helper function
  xfs: rename the next_agno perag iteration variable
  xfs: terminate perag iteration reliably on agcount
  xfs: fix perag reference leak on iteration race with growfs

 fs/xfs/libxfs/xfs_ag.h | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] xfs: fold perag loop iteration logic into helper function

[Brian Foster]

Fold the loop iteration logic into a helper in preparation for
further fixups. No functional change in this patch.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 4c6f9045baca..48eb22e8d717 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -124,12 +124,22 @@ void xfs_perag_put(struct xfs_perag *pag);
  * for_each_perag_from() because they terminate at sb_agcount where there are
  * no perag structures in tree beyond end_agno.
  */
+static inline
+struct xfs_perag *xfs_perag_next(
+	struct xfs_perag	*pag,
+	xfs_agnumber_t		*next_agno)
+{
+	struct xfs_mount	*mp = pag->pag_mount;
+
+	*next_agno = pag->pag_agno + 1;
+	xfs_perag_put(pag);
+	return xfs_perag_get(mp, *next_agno);
+}
+
 #define for_each_perag_range(mp, next_agno, end_agno, pag) \
 	for ((pag) = xfs_perag_get((mp), (next_agno)); \
 		(pag) != NULL && (next_agno) <= (end_agno); \
-		(next_agno) = (pag)->pag_agno + 1, \
-		xfs_perag_put(pag), \
-		(pag) = xfs_perag_get((mp), (next_agno)))
+		(pag) = xfs_perag_next((pag), &(next_agno)))
 
 #define for_each_perag_from(mp, next_agno, pag) \
 	for_each_perag_range((mp), (next_agno), (mp)->m_sb.sb_agcount, (pag))
-- 
2.31.1

[PATCH v2 2/4] xfs: rename the next_agno perag iteration variable

[Brian Foster]

Rename the next_agno variable to be consistent across the several
iteration macros and shorten line length.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 48eb22e8d717..cf8baae2ba18 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -127,22 +127,22 @@ void xfs_perag_put(struct xfs_perag *pag);
 static inline
 struct xfs_perag *xfs_perag_next(
 	struct xfs_perag	*pag,
-	xfs_agnumber_t		*next_agno)
+	xfs_agnumber_t		*agno)
 {
 	struct xfs_mount	*mp = pag->pag_mount;
 
-	*next_agno = pag->pag_agno + 1;
+	*agno = pag->pag_agno + 1;
 	xfs_perag_put(pag);
-	return xfs_perag_get(mp, *next_agno);
+	return xfs_perag_get(mp, *agno);
 }
 
-#define for_each_perag_range(mp, next_agno, end_agno, pag) \
-	for ((pag) = xfs_perag_get((mp), (next_agno)); \
-		(pag) != NULL && (next_agno) <= (end_agno); \
-		(pag) = xfs_perag_next((pag), &(next_agno)))
+#define for_each_perag_range(mp, agno, end_agno, pag) \
+	for ((pag) = xfs_perag_get((mp), (agno)); \
+		(pag) != NULL && (agno) <= (end_agno); \
+		(pag) = xfs_perag_next((pag), &(agno)))
 
-#define for_each_perag_from(mp, next_agno, pag) \
-	for_each_perag_range((mp), (next_agno), (mp)->m_sb.sb_agcount, (pag))
+#define for_each_perag_from(mp, agno, pag) \
+	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount, (pag))
 
 
 #define for_each_perag(mp, agno, pag) \
-- 
2.31.1

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

[Brian Foster]

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.

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

[PATCH v2 4/4] xfs: fix perag reference leak on iteration race with growfs

[Brian Foster]

The for_each_perag*() set of macros are hacky in that some (i.e.
those based on sb_agcount) rely on the assumption that perag
iteration terminates naturally with a NULL perag at the specified
end_agno. Others allow for the final AG to have a valid perag and
require the calling function to clean up any potential leftover
xfs_perag reference on termination of the loop.

Aside from providing a subtly inconsistent interface, the former
variant is racy with growfs because growfs can create discoverable
post-eofs perags before the final superblock update that completes
the grow operation and increases sb_agcount. This leads to the
following assert failure (reproduced by xfs/104) in the perag free
path during unmount:

 XFS: Assertion failed: atomic_read(&pag->pag_ref) == 0, file: fs/xfs/libxfs/xfs_ag.c, line: 195

This occurs because one of the many for_each_perag() loops in the
code that is expected to terminate with a NULL pag (and thus has no
post-loop xfs_perag_put() check) raced with a growfs and found a
non-NULL post-EOFS perag, but terminated naturally based on the
end_agno check without releasing the post-EOFS perag.

Rework the iteration logic to lift the agno check from the main for
loop conditional to the iteration helper function. The for loop now
purely terminates on a NULL pag and xfs_perag_next() avoids taking a
reference to any perag beyond end_agno in the first place.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index b8cc5017efba..e20575c898f9 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -116,30 +116,26 @@ void xfs_perag_put(struct xfs_perag *pag);
 
 /*
  * Perag iteration APIs
- *
- * XXX: for_each_perag_range() usage really needs an iterator to clean up when
- * we terminate at end_agno because we may have taken a reference to the perag
- * beyond end_agno. Right now callers have to be careful to catch and clean that
- * up themselves. This is not necessary for the callers of for_each_perag() and
- * for_each_perag_from() because they terminate at sb_agcount where there are
- * no perag structures in tree beyond end_agno.
  */
 static inline
 struct xfs_perag *xfs_perag_next(
 	struct xfs_perag	*pag,
-	xfs_agnumber_t		*agno)
+	xfs_agnumber_t		*agno,
+	xfs_agnumber_t		end_agno)
 {
 	struct xfs_mount	*mp = pag->pag_mount;
 
 	*agno = pag->pag_agno + 1;
 	xfs_perag_put(pag);
+	if (*agno > end_agno)
+		return NULL;
 	return xfs_perag_get(mp, *agno);
 }
 
 #define for_each_perag_range(mp, agno, end_agno, pag) \
 	for ((pag) = xfs_perag_get((mp), (agno)); \
-		(pag) != NULL && (agno) <= (end_agno); \
-		(pag) = xfs_perag_next((pag), &(agno)))
+		(pag) != NULL; \
+		(pag) = xfs_perag_next((pag), &(agno), (end_agno)))
 
 #define for_each_perag_from(mp, agno, pag) \
 	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1, (pag))
-- 
2.31.1

Re: [PATCH v2 1/4] xfs: fold perag loop iteration logic into helper function

[Darrick J. Wong]

On Tue, Oct 12, 2021 at 12:52:00PM -0400, Brian Foster wrote:
> Fold the loop iteration logic into a helper in preparation for
> further fixups. No functional change in this patch.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.h | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 4c6f9045baca..48eb22e8d717 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -124,12 +124,22 @@ void xfs_perag_put(struct xfs_perag *pag);
>   * for_each_perag_from() because they terminate at sb_agcount where there are
>   * no perag structures in tree beyond end_agno.
>   */
> +static inline
> +struct xfs_perag *xfs_perag_next(

Dorky style nit: function name goes at the start of the line.

static inline struct xfs_perag *
xfs_perag_next(

With that fixed,

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +	struct xfs_perag	*pag,
> +	xfs_agnumber_t		*next_agno)
> +{
> +	struct xfs_mount	*mp = pag->pag_mount;
> +
> +	*next_agno = pag->pag_agno + 1;
> +	xfs_perag_put(pag);
> +	return xfs_perag_get(mp, *next_agno);
> +}
> +
>  #define for_each_perag_range(mp, next_agno, end_agno, pag) \
>  	for ((pag) = xfs_perag_get((mp), (next_agno)); \
>  		(pag) != NULL && (next_agno) <= (end_agno); \
> -		(next_agno) = (pag)->pag_agno + 1, \
> -		xfs_perag_put(pag), \
> -		(pag) = xfs_perag_get((mp), (next_agno)))
> +		(pag) = xfs_perag_next((pag), &(next_agno)))
>  
>  #define for_each_perag_from(mp, next_agno, pag) \
>  	for_each_perag_range((mp), (next_agno), (mp)->m_sb.sb_agcount, (pag))
> -- 
> 2.31.1
> 

Re: [PATCH v2 2/4] xfs: rename the next_agno perag iteration variable

[Darrick J. Wong]

On Tue, Oct 12, 2021 at 12:52:01PM -0400, Brian Foster wrote:
> Rename the next_agno variable to be consistent across the several
> iteration macros and shorten line length.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_ag.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 48eb22e8d717..cf8baae2ba18 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -127,22 +127,22 @@ void xfs_perag_put(struct xfs_perag *pag);
>  static inline
>  struct xfs_perag *xfs_perag_next(
>  	struct xfs_perag	*pag,
> -	xfs_agnumber_t		*next_agno)
> +	xfs_agnumber_t		*agno)
>  {
>  	struct xfs_mount	*mp = pag->pag_mount;
>  
> -	*next_agno = pag->pag_agno + 1;
> +	*agno = pag->pag_agno + 1;
>  	xfs_perag_put(pag);
> -	return xfs_perag_get(mp, *next_agno);
> +	return xfs_perag_get(mp, *agno);
>  }
>  
> -#define for_each_perag_range(mp, next_agno, end_agno, pag) \
> -	for ((pag) = xfs_perag_get((mp), (next_agno)); \
> -		(pag) != NULL && (next_agno) <= (end_agno); \
> -		(pag) = xfs_perag_next((pag), &(next_agno)))
> +#define for_each_perag_range(mp, agno, end_agno, pag) \
> +	for ((pag) = xfs_perag_get((mp), (agno)); \
> +		(pag) != NULL && (agno) <= (end_agno); \
> +		(pag) = xfs_perag_next((pag), &(agno)))
>  
> -#define for_each_perag_from(mp, next_agno, pag) \
> -	for_each_perag_range((mp), (next_agno), (mp)->m_sb.sb_agcount, (pag))
> +#define for_each_perag_from(mp, agno, pag) \
> +	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount, (pag))
>  
>  
>  #define for_each_perag(mp, agno, pag) \
> -- 
> 2.31.1
> 

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

[Darrick J. Wong]

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?

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.

Welcome back, by the way. :)

--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
> 

Re: [PATCH v2 4/4] xfs: fix perag reference leak on iteration race with growfs

[Darrick J. Wong]

On Tue, Oct 12, 2021 at 12:52:03PM -0400, Brian Foster wrote:
> The for_each_perag*() set of macros are hacky in that some (i.e.
> those based on sb_agcount) rely on the assumption that perag
> iteration terminates naturally with a NULL perag at the specified
> end_agno. Others allow for the final AG to have a valid perag and
> require the calling function to clean up any potential leftover
> xfs_perag reference on termination of the loop.
> 
> Aside from providing a subtly inconsistent interface, the former
> variant is racy with growfs because growfs can create discoverable
> post-eofs perags before the final superblock update that completes
> the grow operation and increases sb_agcount. This leads to the
> following assert failure (reproduced by xfs/104) in the perag free
> path during unmount:
> 
>  XFS: Assertion failed: atomic_read(&pag->pag_ref) == 0, file: fs/xfs/libxfs/xfs_ag.c, line: 195
> 
> This occurs because one of the many for_each_perag() loops in the
> code that is expected to terminate with a NULL pag (and thus has no
> post-loop xfs_perag_put() check) raced with a growfs and found a
> non-NULL post-EOFS perag, but terminated naturally based on the
> end_agno check without releasing the post-EOFS perag.
> 
> Rework the iteration logic to lift the agno check from the main for
> loop conditional to the iteration helper function. The for loop now
> purely terminates on a NULL pag and xfs_perag_next() avoids taking a
> reference to any perag beyond end_agno in the first place.

That /definitely/ sounds like it needs a Fixes tag.

With that fixed, I think this is ready to go:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.h | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index b8cc5017efba..e20575c898f9 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -116,30 +116,26 @@ void xfs_perag_put(struct xfs_perag *pag);
>  
>  /*
>   * Perag iteration APIs
> - *
> - * XXX: for_each_perag_range() usage really needs an iterator to clean up when
> - * we terminate at end_agno because we may have taken a reference to the perag
> - * beyond end_agno. Right now callers have to be careful to catch and clean that
> - * up themselves. This is not necessary for the callers of for_each_perag() and
> - * for_each_perag_from() because they terminate at sb_agcount where there are
> - * no perag structures in tree beyond end_agno.
>   */
>  static inline
>  struct xfs_perag *xfs_perag_next(
>  	struct xfs_perag	*pag,
> -	xfs_agnumber_t		*agno)
> +	xfs_agnumber_t		*agno,
> +	xfs_agnumber_t		end_agno)
>  {
>  	struct xfs_mount	*mp = pag->pag_mount;
>  
>  	*agno = pag->pag_agno + 1;
>  	xfs_perag_put(pag);
> +	if (*agno > end_agno)
> +		return NULL;
>  	return xfs_perag_get(mp, *agno);
>  }
>  
>  #define for_each_perag_range(mp, agno, end_agno, pag) \
>  	for ((pag) = xfs_perag_get((mp), (agno)); \
> -		(pag) != NULL && (agno) <= (end_agno); \
> -		(pag) = xfs_perag_next((pag), &(agno)))
> +		(pag) != NULL; \
> +		(pag) = xfs_perag_next((pag), &(agno), (end_agno)))
>  
>  #define for_each_perag_from(mp, agno, pag) \
>  	for_each_perag_range((mp), (agno), (mp)->m_sb.sb_agcount - 1, (pag))
> -- 
> 2.31.1
> 

Re: [PATCH v2 0/4] xfs: fix perag iteration raciness

[Dave Chinner]

On Tue, Oct 12, 2021 at 12:51:59PM -0400, Brian Foster wrote:
> v2:
> - Factoring and patch granularity.
> v1: https://lore.kernel.org/linux-xfs/20211007125053.1096868-1-bfoster@redhat.com/
> 
> Brian Foster (4):
>   xfs: fold perag loop iteration logic into helper function
>   xfs: rename the next_agno perag iteration variable
>   xfs: terminate perag iteration reliably on agcount
>   xfs: fix perag reference leak on iteration race with growfs

Looks good now. For the entire series:

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Brian Foster]

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
> > 
> 

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

[Darrick J. Wong]

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

> > 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@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
> > > 
> > 
> 

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

[Brian Foster]

On Thu, Oct 14, 2021 at 09:46:21AM -0700, Darrick J. Wong wrote:
> 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@kernel.org>
> 

Thanks. Do you want me to post my v3 with the style and tag fixes or
have you already made those changes?

> > > 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.
> 

Oh, I see. Yeah, I think technically that would be more defensive logic.
We might be able to repurpose xfs_perag_next() into something more
generic that also covers the init case, but I'm not terribly concerned
with that type of misuse in the context of this patch (and not sure it
warrants the quirky logic if there are bigger changes pending with these
macros anyways).

Brian

> --D
> 
> > > 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
> > > > 
> > > 
> > 
> 

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

[Darrick J. Wong]

On Thu, Oct 14, 2021 at 01:41:57PM -0400, Brian Foster wrote:
> On Thu, Oct 14, 2021 at 09:46:21AM -0700, Darrick J. Wong wrote:
> > 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@kernel.org>
> > 
> 
> Thanks. Do you want me to post my v3 with the style and tag fixes or
> have you already made those changes?

Yes please. :)

> > > > 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.
> > 
> 
> Oh, I see. Yeah, I think technically that would be more defensive logic.
> We might be able to repurpose xfs_perag_next() into something more
> generic that also covers the init case, but I'm not terribly concerned
> with that type of misuse in the context of this patch (and not sure it
> warrants the quirky logic if there are bigger changes pending with these
> macros anyways).

Nah, I think we can probably let that go.  If one of us trips over this
6mo from now when we've forgotten, we can add the extra training wheels
for ourselves at that time. ;)

--D

> 
> Brian
> 
> > --D
> > 
> > > > 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
> > > > > 
> > > > 
> > > 
> > 
>