* [PATCH v3 0/4] xfs: fix perag iteration raciness
@ 2021-10-14 17:58 Brian Foster
2021-10-14 17:58 ` [PATCH v3 1/4] xfs: fold perag loop iteration logic into helper function Brian Foster
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Brian Foster @ 2021-10-14 17:58 UTC (permalink / raw)
To: linux-xfs
v3:
- Code style, Fixes: and RvB: tags.
v2: https://lore.kernel.org/linux-xfs/20211012165203.1354826-1-bfoster@redhat.com/
- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/4] xfs: fold perag loop iteration logic into helper function
2021-10-14 17:58 [PATCH v3 0/4] xfs: fix perag iteration raciness Brian Foster
@ 2021-10-14 17:58 ` Brian Foster
2021-10-14 17:59 ` [PATCH v3 2/4] xfs: rename the next_agno perag iteration variable Brian Foster
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Brian Foster @ 2021-10-14 17:58 UTC (permalink / raw)
To: linux-xfs
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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
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..ddb89e10b6ea 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/4] xfs: rename the next_agno perag iteration variable
2021-10-14 17:58 [PATCH v3 0/4] xfs: fix perag iteration raciness Brian Foster
2021-10-14 17:58 ` [PATCH v3 1/4] xfs: fold perag loop iteration logic into helper function Brian Foster
@ 2021-10-14 17:59 ` Brian Foster
2021-10-14 17:59 ` [PATCH v3 3/4] xfs: terminate perag iteration reliably on agcount Brian Foster
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Brian Foster @ 2021-10-14 17:59 UTC (permalink / raw)
To: linux-xfs
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: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
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 ddb89e10b6ea..134e8635dee1 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 3/4] xfs: terminate perag iteration reliably on agcount
2021-10-14 17:58 [PATCH v3 0/4] xfs: fix perag iteration raciness Brian Foster
2021-10-14 17:58 ` [PATCH v3 1/4] xfs: fold perag loop iteration logic into helper function Brian Foster
2021-10-14 17:59 ` [PATCH v3 2/4] xfs: rename the next_agno perag iteration variable Brian Foster
@ 2021-10-14 17:59 ` Brian Foster
2021-10-14 17:59 ` [PATCH v3 4/4] xfs: fix perag reference leak on iteration race with growfs Brian Foster
2021-10-14 20:16 ` [PATCH v3 0/4] xfs: fix perag iteration raciness Darrick J. Wong
4 siblings, 0 replies; 6+ messages in thread
From: Brian Foster @ 2021-10-14 17:59 UTC (permalink / raw)
To: linux-xfs
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.
Fixes: 58d43a7e3263 ("xfs: pass perags around in fsmap data dev functions")
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
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 134e8635dee1..4585ebb3f450 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -142,7 +142,7 @@ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 4/4] xfs: fix perag reference leak on iteration race with growfs
2021-10-14 17:58 [PATCH v3 0/4] xfs: fix perag iteration raciness Brian Foster
` (2 preceding siblings ...)
2021-10-14 17:59 ` [PATCH v3 3/4] xfs: terminate perag iteration reliably on agcount Brian Foster
@ 2021-10-14 17:59 ` Brian Foster
2021-10-14 20:16 ` [PATCH v3 0/4] xfs: fix perag iteration raciness Darrick J. Wong
4 siblings, 0 replies; 6+ messages in thread
From: Brian Foster @ 2021-10-14 17:59 UTC (permalink / raw)
To: linux-xfs
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.
Fixes: f250eedcf762 ("xfs: make for_each_perag... a first class citizen")
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
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 4585ebb3f450..3f597cad2c33 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/4] xfs: fix perag iteration raciness
2021-10-14 17:58 [PATCH v3 0/4] xfs: fix perag iteration raciness Brian Foster
` (3 preceding siblings ...)
2021-10-14 17:59 ` [PATCH v3 4/4] xfs: fix perag reference leak on iteration race with growfs Brian Foster
@ 2021-10-14 20:16 ` Darrick J. Wong
4 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2021-10-14 20:16 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Thu, Oct 14, 2021 at 01:58:58PM -0400, Brian Foster wrote:
> v3:
> - Code style, Fixes: and RvB: tags.
> v2: https://lore.kernel.org/linux-xfs/20211012165203.1354826-1-bfoster@redhat.com/
> - Factoring and patch granularity.
> v1: https://lore.kernel.org/linux-xfs/20211007125053.1096868-1-bfoster@redhat.com/
Applied, thanks!
--D
>
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-14 20:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14 17:58 [PATCH v3 0/4] xfs: fix perag iteration raciness Brian Foster
2021-10-14 17:58 ` [PATCH v3 1/4] xfs: fold perag loop iteration logic into helper function Brian Foster
2021-10-14 17:59 ` [PATCH v3 2/4] xfs: rename the next_agno perag iteration variable Brian Foster
2021-10-14 17:59 ` [PATCH v3 3/4] xfs: terminate perag iteration reliably on agcount Brian Foster
2021-10-14 17:59 ` [PATCH v3 4/4] xfs: fix perag reference leak on iteration race with growfs Brian Foster
2021-10-14 20:16 ` [PATCH v3 0/4] xfs: fix perag iteration raciness Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).