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

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

On Mon, Dec 11, 2023 at 02:03:05PM -0800, Darrick J. Wong wrote:
> On Sat, Dec 09, 2023 at 08:21:07PM +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
> > 
> > Now, the logic for freeing perag in xfs_free_perag() and
> > xfs_initialize_perag() error handling is essentially the same. Factor
> > out xfs_free_perag_range() from xfs_free_perag(), used for freeing
> > unused perag within a specified range, inclued when growfs fails.
> > 
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  fs/xfs/libxfs/xfs_ag.c | 35 ++++++++++++++++++++---------------
> >  fs/xfs/libxfs/xfs_ag.h |  2 ++
> >  fs/xfs/xfs_fsops.c     |  5 ++++-
> >  3 files changed, 26 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 11ed048c350c..edec03ab09aa 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -245,16 +245,20 @@ __xfs_free_perag(
> >  }
> >  
> >  /*
> > - * Free up the per-ag resources associated with the mount structure.
> > + * Free per-ag within the specified range, if agno is not found in the
> > + * radix tree, then it means that agno and subsequent AGs have not been
> > + * initialized.
> >   */
> >  void
> > -xfs_free_perag(
> > -	struct xfs_mount	*mp)
> > +xfs_free_perag_range(
> > +		xfs_mount_t		*mp,
> 
> Please stop reverting the codebase's use of typedefs for struct
> pointers.
> 
> > +		xfs_agnumber_t		agstart,
> > +		xfs_agnumber_t		agend)
> 
> This is also ^^ unnecessary indentation.
> 
> >  {
> > -	struct xfs_perag	*pag;
> >  	xfs_agnumber_t		agno;
> > +	struct xfs_perag	*pag;
> 
> ...and unnecessary rearranging of variables...

I ignored these minor issues, thanks for pointing them out, and it will be changed
in the next version.

Thanks,
Long Li