Re: [PATCH v2] xfs: fix ag count overflow during growfs

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

On Fri, Apr 28, 2023 at 11:24:52AM -0700, Darrick J. Wong wrote:
> On Fri, Apr 28, 2023 at 03:20:12PM +0800, Long Li wrote:
> > I found a corruption during growfs:
> > 
> >  XFS (loop0): Internal error agbno >= mp->m_sb.sb_agblocks at line 3661 of
> >    file fs/xfs/libxfs/xfs_alloc.c.  Caller __xfs_free_extent+0x28e/0x3c0
> >  CPU: 0 PID: 573 Comm: xfs_growfs Not tainted 6.3.0-rc7-next-20230420-00001-gda8c95746257
> >  Call Trace:
> >   <TASK>
> >   dump_stack_lvl+0x50/0x70
> >   xfs_corruption_error+0x134/0x150
> >   __xfs_free_extent+0x2c1/0x3c0
> >   xfs_ag_extend_space+0x291/0x3e0
> >   xfs_growfs_data+0xd72/0xe90
> >   xfs_file_ioctl+0x5f9/0x14a0
> >   __x64_sys_ioctl+0x13e/0x1c0
> >   do_syscall_64+0x39/0x80
> >   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >  XFS (loop0): Corruption detected. Unmount and run xfs_repair
> >  XFS (loop0): Internal error xfs_trans_cancel at line 1097 of file
> >    fs/xfs/xfs_trans.c.  Caller xfs_growfs_data+0x691/0xe90
> >  CPU: 0 PID: 573 Comm: xfs_growfs Not tainted 6.3.0-rc7-next-20230420-00001-gda8c95746257
> >  Call Trace:
> >   <TASK>
> >   dump_stack_lvl+0x50/0x70
> >   xfs_error_report+0x93/0xc0
> >   xfs_trans_cancel+0x2c0/0x350
> >   xfs_growfs_data+0x691/0xe90
> >   xfs_file_ioctl+0x5f9/0x14a0
> >   __x64_sys_ioctl+0x13e/0x1c0
> >   do_syscall_64+0x39/0x80
> >   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >  RIP: 0033:0x7f2d86706577
> > 
> > The bug can be reproduced with the following sequence:
> > 
> >  # truncate -s  1073741824 xfs_test.img
> >  # mkfs.xfs -f -b size=1024 -d agcount=4 xfs_test.img
> >  # truncate -s 2305843009213693952  xfs_test.img
> >  # mount -o loop xfs_test.img /mnt/test
> >  # xfs_growfs -D  1125899907891200  /mnt/test
> > 
> > The root cause is that during growfs, user space passed in a large value
> > of newblcoks to xfs_growfs_data_private(), due to current sb_agblocks is
> > too small, new AG count will exceed UINT_MAX. Because of AG number type
> > is unsigned int and it would overflow, that caused nagcount much smaller
> > than the actual value. During AG extent space, delta blocks in
> > xfs_resizefs_init_new_ags() will much larger than the actual value due to
> > incorrect nagcount, even exceed UINT_MAX. This will cause corruption and
> > be detected in __xfs_free_extent. Fix it by add checks for nagcount
> > overflow in xfs_growfs_data_private.
> > 
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> > v2:
> > - Check for overflowing of agcount only in xfs_growfs_data_private
> > 
> >  fs/xfs/xfs_fsops.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 13851c0d640b..084c69a91937 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -116,6 +116,9 @@ xfs_growfs_data_private(
> >  	nb_div = nb;
> >  	nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks);
> >  	nagcount = nb_div + (nb_mod != 0);
> > +	/* check for overflow */
> > +	if (nagcount < nb_div)
> > +		return -EINVAL;
> >  	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
> >  		nagcount--;
> 
> If in->newblocks (aka nb) is just large enough to cause an overflow in
> nagcount /and/ 0 < nb_mod < XFS_MIN_AG_BLOCKS, then this change would
> make the function return EINVAL whereas before it would've succeeded
> because the overflow from the division would be canceled out by the
> underflow from the subtraction, right?
> 
Yes, you are right. The behavior before and after the modification needs
to be consistent in this corner case.

> Granted, that's a corner case of a corner case, but I don't want to
> introduce error returns where there previously were none.
> 
> Also, do we want to return EINVAL here, as opposed to growing the
> filesystem to up to the maximally allowed 0xFFFFFFFF AGs?

I think there is no problem not returning EINVAL, your opinion is more important. :)

> 
> 	#define XFS_AGNUMBER_MAX	((xfs_agnumber_t)(-1U))
> 	u64 nb_div = nb;
> 
> 	/* nb_div is updated in place */
> 	nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks);
> 	if (nb_mod && nb_mod >= XFS_MIN_AG_BLOCKS) {
> 		nb_div++;
> 	} else if (nb_mod) {
> 		nb = nb_div * mp->m_sb.sb_agblocks;
> 	}
> 	if (nb_div > XFS_AGNUMBER_MAX) {
> 		nb_div = XFS_AGNUMBER_MAX;
> 		nb = min(nb, nb_div * mp->m_sb.sb_agblocks);

I don't think min() is needed here, if nb_div > XFS_AGNUMBER_MAX is true,
nb_div can only be added by 1 when nb_mod >= XFS_MIN_AG_BLOCKS, the
following expression must be true: 

	nb > (XFS_AGNUMBER_MAX * mp->m_sb.sb_agblocks)

So XFS_AGNUMBER_MAX * mp->m_sb.sb_agblocks must be minimal.

Thank,
Long Li

> 	}
> 	nagcount = nb_div;
> 	delta = nb - mp->m_sb.sb_dblocks;
> 
> --D
> 
> >  		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
> > -- 
> > 2.31.1
> >