[PATCH 1/2] xfs: add lock protection when remove perag from radix tree

[PATCH 1/2] xfs: add lock protection when remove perag from radix tree

[Long Li]

Look at the perag insertion into the radix tree, protected by
mp->m_perag_lock. When the file system is unmounted, the perag is
removed from the radix tree, also protected by mp->m_perag_lock.
Therefore, mp->m_perag_lock is also added when removing a perag
from the radix tree in error path in xfs_initialize_perag().

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/libxfs/xfs_ag.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index f9f4d694640d..cc10a3ca052f 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -424,13 +424,17 @@ xfs_initialize_perag(
 
 out_remove_pag:
 	xfs_defer_drain_free(&pag->pag_intents_drain);
+	spin_lock(&mp->m_perag_lock);
 	radix_tree_delete(&mp->m_perag_tree, index);
+	spin_unlock(&mp->m_perag_lock);
 out_free_pag:
 	kmem_free(pag);
 out_unwind_new_pags:
 	/* unwind any prior newly initialized pags */
 	for (index = first_initialised; index < agcount; index++) {
+		spin_lock(&mp->m_perag_lock);
 		pag = radix_tree_delete(&mp->m_perag_tree, index);
+		spin_unlock(&mp->m_perag_lock);
 		if (!pag)
 			break;
 		xfs_buf_hash_destroy(pag);
-- 
2.31.1

[PATCH 2/2] xfs: fix perag leak when growfs fails

[Long Li]

During growfs, if new ag in memory has been initialized, however sb_agcount
has not been updated, if an error occurs at this time it will cause ag
leaks as follows, these new ags will not been freed during umount because
of sb_agcount is not been updated.

unreferenced object 0xffff88810be40200 (size 512):
  comm "xfs_growfs", pid 857, jiffies 4294909093
  hex dump (first 32 bytes):
    00 c0 c1 05 81 88 ff ff 04 00 00 00 00 00 00 00  ................
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace (crc 381741e2):
    [<ffffffff8191aef6>] __kmalloc+0x386/0x4f0
    [<ffffffff82553e65>] kmem_alloc+0xb5/0x2f0
    [<ffffffff8238dac5>] xfs_initialize_perag+0xc5/0x810
    [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0
    [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0
    [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0
    [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0
    [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a
unreferenced object 0xffff88810be40800 (size 512):
  comm "xfs_growfs", pid 857, jiffies 4294909093
  hex dump (first 32 bytes):
    20 00 00 00 00 00 00 00 57 ef be dc 00 00 00 00   .......W.......
    10 08 e4 0b 81 88 ff ff 10 08 e4 0b 81 88 ff ff  ................
  backtrace (crc bde50e2d):
    [<ffffffff8191b43a>] __kmalloc_node+0x3da/0x540
    [<ffffffff81814489>] kvmalloc_node+0x99/0x160
    [<ffffffff8286acff>] bucket_table_alloc.isra.0+0x5f/0x400
    [<ffffffff8286bdc5>] rhashtable_init+0x405/0x760
    [<ffffffff8238dda3>] xfs_initialize_perag+0x3a3/0x810
    [<ffffffff824f679c>] xfs_growfs_data+0x9bc/0xbc0
    [<ffffffff8250b90e>] xfs_file_ioctl+0x5fe/0x14d0
    [<ffffffff81aa5194>] __x64_sys_ioctl+0x144/0x1c0
    [<ffffffff83c3d81f>] do_syscall_64+0x3f/0xe0
    [<ffffffff83e00087>] entry_SYSCALL_64_after_hwframe+0x62/0x6a

Factor out xfs_destroy_perag() from xfs_initialize_perag() for error
handle. When growfs fails, use xfs_destroy_perag() to destroy newly
initialized ag in error handle path.

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/libxfs/xfs_ag.c | 32 ++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_ag.h |  2 ++
 fs/xfs/xfs_fsops.c     |  5 ++++-
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index cc10a3ca052f..5c2a7402e526 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -332,6 +332,27 @@ xfs_agino_range(
 	return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last);
 }
 
+void
+xfs_destroy_perag(
+	xfs_mount_t		*mp,
+	xfs_agnumber_t		agstart,
+	xfs_agnumber_t		agend)
+{
+	xfs_agnumber_t		index;
+	struct xfs_perag	*pag;
+
+	for (index = agstart; index < agend; index++) {
+		spin_lock(&mp->m_perag_lock);
+		pag = radix_tree_delete(&mp->m_perag_tree, index);
+		spin_unlock(&mp->m_perag_lock);
+		if (!pag)
+			break;
+		xfs_buf_hash_destroy(pag);
+		xfs_defer_drain_free(&pag->pag_intents_drain);
+		kmem_free(pag);
+	}
+}
+
 int
 xfs_initialize_perag(
 	struct xfs_mount	*mp,
@@ -431,16 +452,7 @@ xfs_initialize_perag(
 	kmem_free(pag);
 out_unwind_new_pags:
 	/* unwind any prior newly initialized pags */
-	for (index = first_initialised; index < agcount; index++) {
-		spin_lock(&mp->m_perag_lock);
-		pag = radix_tree_delete(&mp->m_perag_tree, index);
-		spin_unlock(&mp->m_perag_lock);
-		if (!pag)
-			break;
-		xfs_buf_hash_destroy(pag);
-		xfs_defer_drain_free(&pag->pag_intents_drain);
-		kmem_free(pag);
-	}
+	xfs_destroy_perag(mp, first_initialised, agcount);
 	return error;
 }
 
diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 2e0aef87d633..142abdc50331 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -133,6 +133,8 @@ __XFS_AG_OPSTATE(prefers_metadata, PREFERS_METADATA)
 __XFS_AG_OPSTATE(allows_inodes, ALLOWS_INODES)
 __XFS_AG_OPSTATE(agfl_needs_reset, AGFL_NEEDS_RESET)
 
+void xfs_destroy_perag(xfs_mount_t *mp, xfs_agnumber_t agstart,
+			xfs_agnumber_t agend);
 int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount,
 			xfs_rfsblock_t dcount, xfs_agnumber_t *maxagi);
 int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno);
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 57076a25f17d..385ef3a23e2a 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -153,7 +153,7 @@ xfs_growfs_data_private(
 		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, -delta, 0,
 				0, &tp);
 	if (error)
-		return error;
+		goto destroy_perag;
 
 	last_pag = xfs_perag_get(mp, oagcount - 1);
 	if (delta > 0) {
@@ -227,6 +227,9 @@ xfs_growfs_data_private(
 
 out_trans_cancel:
 	xfs_trans_cancel(tp);
+destroy_perag:
+	if (nagcount > oagcount)
+		xfs_destroy_perag(mp, oagcount, nagcount);
 	return error;
 }
 
-- 
2.31.1

Re: [PATCH 1/2] xfs: add lock protection when remove perag from radix tree

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 12:39:10PM +0800, Long Li wrote:
> Look at the perag insertion into the radix tree, protected by
> mp->m_perag_lock. When the file system is unmounted, the perag is
> removed from the radix tree, also protected by mp->m_perag_lock.
> Therefore, mp->m_perag_lock is also added when removing a perag
> from the radix tree in error path in xfs_initialize_perag().

There really can't be anything we are racing with at this point.
That beeing said I think clearing locking rules are always a good
thing.  So maybe reword the above as:

"Take mp->m_perag_lock for deletions from the perag radix tree in
 xfs_initialize_perag to be consistent with additions to it even
 if there can't be concurrent modifications to it at this point"

With that:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: fix perag leak when growfs fails

[Christoph Hellwig]

> +void
> +xfs_destroy_perag(
> +	xfs_mount_t		*mp,
> +	xfs_agnumber_t		agstart,
> +	xfs_agnumber_t		agend)

Not sure xfs_destroy_perag is the right name, as it frees a range of
AGs, how about xfs_free_unuses_perag_range?

Also a comment explaining that this must never be used for AGs that
have been visible (that is included in mp->m_sb.sb_agcount) would
probably be useful.

Re: [PATCH 1/2] xfs: add lock protection when remove perag from radix tree

[Dave Chinner]

On Mon, Dec 04, 2023 at 08:35:08PM -0800, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 12:39:10PM +0800, Long Li wrote:
> > Look at the perag insertion into the radix tree, protected by
> > mp->m_perag_lock. When the file system is unmounted, the perag is
> > removed from the radix tree, also protected by mp->m_perag_lock.
> > Therefore, mp->m_perag_lock is also added when removing a perag
> > from the radix tree in error path in xfs_initialize_perag().
> 
> There really can't be anything we are racing with at this point.

I'm pretty sure that there can be racing operations. Lookups are
fine - they are RCU protected so already deal with the tree changing
shape underneath the lookup - but tagging operations require the
tree to be stable while the tags are propagated back up to the root.

Right now there's nothing stopping radix tree tagging from
operating while a growfs operation is progress and adding/removing
new entries into the radix tree.

Hence we can have traversals that require a stable tree occurring at
the same time we are removing unused entries from the radix tree
which causes the shape of the tree to change...

Likely this hasn't caused a problem in the past because we are only
doing append addition and removal so the active AG part of the tree
is not changing shape, but that doesn't mean it is safe...

> That beeing said I think clearing locking rules are always a good
> thing.  So maybe reword the above as:
> 
> "Take mp->m_perag_lock for deletions from the perag radix tree in
>  xfs_initialize_perag to be consistent with additions to it even
>  if there can't be concurrent modifications to it at this point"

I don't think it needs even that - just making the radix tree
modifications serialise against each other is obviously correct...

-Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfs: add lock protection when remove perag from radix tree

[Long Li]

On Wed, Dec 06, 2023 at 08:28:00AM +1100, Dave Chinner wrote:
> On Mon, Dec 04, 2023 at 08:35:08PM -0800, Christoph Hellwig wrote:
> > On Mon, Dec 04, 2023 at 12:39:10PM +0800, Long Li wrote:
> > > Look at the perag insertion into the radix tree, protected by
> > > mp->m_perag_lock. When the file system is unmounted, the perag is
> > > removed from the radix tree, also protected by mp->m_perag_lock.
> > > Therefore, mp->m_perag_lock is also added when removing a perag
> > > from the radix tree in error path in xfs_initialize_perag().
> > 
> > There really can't be anything we are racing with at this point.
> 
> I'm pretty sure that there can be racing operations. Lookups are
> fine - they are RCU protected so already deal with the tree changing
> shape underneath the lookup - but tagging operations require the
> tree to be stable while the tags are propagated back up to the root.
> 
> Right now there's nothing stopping radix tree tagging from
> operating while a growfs operation is progress and adding/removing
> new entries into the radix tree.
> 
> Hence we can have traversals that require a stable tree occurring at
> the same time we are removing unused entries from the radix tree
> which causes the shape of the tree to change...
> 
> Likely this hasn't caused a problem in the past because we are only
> doing append addition and removal so the active AG part of the tree
> is not changing shape, but that doesn't mean it is safe...
> 
> > That beeing said I think clearing locking rules are always a good
> > thing.  So maybe reword the above as:
> > 
> > "Take mp->m_perag_lock for deletions from the perag radix tree in
> >  xfs_initialize_perag to be consistent with additions to it even
> >  if there can't be concurrent modifications to it at this point"
> 
> I don't think it needs even that - just making the radix tree
> modifications serialise against each other is obviously correct...
> 

If my understanding is correct, it makes sense to add lock protection
when modifying the radix tree. So I will update the commit message in
the next version.

Thanks,
Long Li

Re: [PATCH 2/2] xfs: fix perag leak when growfs fails

[Long Li]

On Mon, Dec 04, 2023 at 08:38:08PM -0800, Christoph Hellwig wrote:
> > +void
> > +xfs_destroy_perag(
> > +	xfs_mount_t		*mp,
> > +	xfs_agnumber_t		agstart,
> > +	xfs_agnumber_t		agend)
> 
> Not sure xfs_destroy_perag is the right name, as it frees a range of
> AGs, how about xfs_free_unuses_perag_range?

Yes, it looks better.
But I'm not sure if it's better to use xfs_free_unused_perag_range?

> 
> Also a comment explaining that this must never be used for AGs that
> have been visible (that is included in mp->m_sb.sb_agcount) would
> probably be useful.
> 

This will be changed in the next version.

Thanks,
Long Li