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

[Long Li <leo.lilong@huawei.com>]

On Wed, Dec 13, 2023 at 10:21:12AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 13, 2023 at 11:10:13AM +0800, Long Li wrote:
> > 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 perag leaks as follows, these new AGs will not been freed
> > during umount , because of these new AGs are not visible(that is
> > included in mp->m_sb.sb_agcount).
> > 
> > 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_free_unused_perag_range() from xfs_initialize_perag(),
> > used for freeing unused perag within a specified range in error handling,
> > included in the error path of the growfs failure.
> > 
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> 
> This looks like a fix for a bug; is there a reason you omitted a Fixes:
> tag?  Has this bug been here so long that blame points at the
> granddaddy commit?

Yes, this bug has been around for a long time, and I've looked up the
submission history again.

Long times ago, it used an array for the per-ag structures, per-ag
array can be freed in xfs_free_perag(). After commit 1c1c6ebcf528 ("xfs:
Replace per-ag array with a radix tree") was added, only visible AGs
will be freed, so it need to add the following fix tag.

Fixes: 1c1c6ebcf528 ("xfs: Replace per-ag array with a radix tree")

Thanks.

> 
> Code looks ok to me though.
> 
> --D
> 
> > ---
> > v3:
> > - Stop use of typedefs for struct pointers
> > - Remove unnecessary indentation
> > - Factor out xfs_free_unused_perag_range() from xfs_initialize_perag(),
> >   not xfs_free_perag(). Compared to the v2 version, now the logic for
> >   freeing perag in xfs_free_perag() and xfs_initialize_perag() error
> >   handling not quite the same.
> >  fs/xfs/libxfs/xfs_ag.c | 36 ++++++++++++++++++++++++++----------
> >  fs/xfs/libxfs/xfs_ag.h |  2 ++
> >  fs/xfs/xfs_fsops.c     |  5 ++++-
> >  3 files changed, 32 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index c730976fdfc0..39d9525270b7 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -332,6 +332,31 @@ xfs_agino_range(
> >  	return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last);
> >  }
> >  
> > +/*
> > + * Free perag within the specified AG range, it is only used to free unused
> > + * perags under the error handling path.
> > + */
> > +void
> > +xfs_free_unused_perag_range(
> > +	struct xfs_mount	*mp,
> > +	xfs_agnumber_t		agstart,
> > +	xfs_agnumber_t		agend)
> > +{
> > +	struct xfs_perag	*pag;
> > +	xfs_agnumber_t		index;
> > +
> > +	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 +456,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_free_unused_perag_range(mp, first_initialised, agcount);
> >  	return error;
> >  }
> >  
> > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> > index 2e0aef87d633..40d7b6427afb 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_free_unused_perag_range(struct xfs_mount *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 e8759b479516..22c3f1e9008e 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 out_free_unused_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);
> > +out_free_unused_perag:
> > +	if (nagcount > oagcount)
> > +		xfs_free_unused_perag_range(mp, oagcount, nagcount);
> >  	return error;
> >  }
> >  
> > -- 
> > 2.31.1
> > 
> > 
>