| Message ID | 20211012165203.1354826-5-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: fix perag iteration raciness | expand |
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 >
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))
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(-)