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

["Darrick J. Wong" <djwong@kernel.org>]

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
>