On Tue, Apr 25, 2023 at 08:15:29AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 25, 2023 at 10:53:45AM +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 AG number that
> > should not greater than or equal to NULLAGNUMBER before growfs and mount
> > filesystem.
> >
> > Signed-off-by: Long Li <leo.lilong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_fsops.c | 2 +-
> > fs/xfs/xfs_mount.c | 6 +++++-
> > fs/xfs/xfs_mount.h | 2 +-
> > fs/xfs/xfs_rtalloc.c | 2 +-
> > fs/xfs/xfs_super.c | 4 ++--
> > 5 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 13851c0d640b..0f0b12eaf53a 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -100,7 +100,7 @@ xfs_growfs_data_private(
> > struct xfs_perag *last_pag;
> >
> > nb = in->newblocks;
> > - error = xfs_sb_validate_fsb_count(&mp->m_sb, nb);
> > + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb, true);
> > if (error)
> > return error;
> >
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index fb87ffb48f7f..284c11c1c6e8 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -128,7 +128,8 @@ xfs_uuid_unmount(
> > int
> > xfs_sb_validate_fsb_count(
> > xfs_sb_t *sbp,
> > - uint64_t nblocks)
> > + uint64_t nblocks,
> > + bool dblock)
> > {
> > ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
> > ASSERT(sbp->sb_blocklog >= BBSHIFT);
> > @@ -136,6 +137,9 @@ xfs_sb_validate_fsb_count(
> > /* Limited by ULONG_MAX of page cache index */
> > if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
> > return -EFBIG;
> > + /* Limited by NULLAGNUMBER of ag number */
> > + if (dblock && (nblocks >> sbp->sb_agblklog) >= NULLAGNUMBER)
> > + return -EFBIG;
>
> I think this should be a separate predicate to check for overflowing
> agcount in xfs_validate_sb_common and xfs_growfs_data_private.
>
Ok, the next version will be changed.
> I also wonder if @agcount in xfs_validate_sb_common needs to be u64 (and
> not u32) to handle overflows?
It looks like there is no need for @agcount overflow checking in xfs_validate_sb_common:
If agcount overflow occurs, the flollowing judgment will be true and SB sanity check failed.
sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)
So the check for overflowing of agcount should only need in xfs_growfs_data_private().
Thanks,
Long Li
> Someone should try fuzzing a filesystem with a small agblklog and a
> large dblocks to see if one can trip an integer overflow in the
> superblock verifier.
>
> --D
>
> > return 0;
> > }
> >
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index f3269c0626f0..a69e9b21ef61 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -531,7 +531,7 @@ xfs_mod_frextents(struct xfs_mount *mp, int64_t delta)
> > extern int xfs_readsb(xfs_mount_t *, int);
> > extern void xfs_freesb(xfs_mount_t *);
> > extern bool xfs_fs_writable(struct xfs_mount *mp, int level);
> > -extern int xfs_sb_validate_fsb_count(struct xfs_sb *, uint64_t);
> > +extern int xfs_sb_validate_fsb_count(struct xfs_sb *, uint64_t, bool);
> >
> > extern int xfs_dev_is_read_only(struct xfs_mount *, char *);
> >
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 16534e9873f6..c207026d92ac 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -958,7 +958,7 @@ xfs_growfs_rt(
> > return -EOPNOTSUPP;
> >
> > nrblocks = in->newblocks;
> > - error = xfs_sb_validate_fsb_count(sbp, nrblocks);
> > + error = xfs_sb_validate_fsb_count(sbp, nrblocks, false);
> > if (error)
> > return error;
> > /*
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 4d2e87462ac4..72dfd02c588e 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1592,8 +1592,8 @@ xfs_fs_fill_super(
> > }
> >
> > /* Ensure this filesystem fits in the page cache limits */
> > - if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) ||
> > - xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) {
> > + if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks, true) ||
> > + xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks, false)) {
> > xfs_warn(mp,
> > "file system too large to be mounted on this system.");
> > error = -EFBIG;
> > --
> > 2.31.1
> >
