make growfs check device size limits too
PATCH 1/1 make growfs check device size limits too
On the mount path we check for a superblock that describes a filesystem to large for the running kernel to handle. This catches the case of an attempt to mount a >16TB filesystem on i386 (where we are limited by the page->index size, for XFS metadata buffers in xfs_buf.c).
This patch makes similar checks on the growfs code paths for regular and realtime growth, else we can end up with filesystem corruption, it would seem (from #xfs chatter). Untested patch follows; probably better to do this as a macro, in a header, and call that in each place...?
cheers.
Signature · 1 line
Nathan
xfs/xfs_fsops.c renamed from xfs/xfs_fsops.c.orig +14 -0
@@ -148,6 +148,20 @@return error;ASSERT (bp);xfs_buf_relse (bp); +/* +* Device drivers seem to be pathological liars... so, guess we +* better check that the size isn't something completely insane. +* Same check is done during mount, so we wont create something +* here that we cannot later mount, at least. +*/ +#if XFS_BIG_BLKNOS /* Limited by ULONG_MAX of page cache index */ +if (unlikely ( + (nb >> (PAGE_CACHE_SHIFT - sbp->sb_blocklog)) >ULONG_MAX )) +#else /* Limited by UINT_MAX of sectors */ +if (unlikely ( + (nb << (sbp->sb_blocklog - BBSHIFT)) > UINT_MAX)) +#endif +return XFS_ERROR (E2BIG ); new = nb;/* use new as a temporary here */ nb_mod =do_div (new, mp->m_sb.sb_agblocks);
xfs/xfs_rtalloc.c renamed from xfs/xfs_rtalloc.c.orig +14 -0
@@ -1893,6 +1893,20 @@ASSERT (bp);xfs_buf_relse (bp);/* +* Device drivers seem to be pathological liars... so, guess we +* better check that the size isn't something completely insane. +* Same check is done during mount, so we wont create something +* here that we cannot later mount, at least. +*/ +#if XFS_BIG_BLKNOS /* Limited by ULONG_MAX of page cache index */ +if (unlikely ( + (nrblocks >> (PAGE_CACHE_SHIFT - sbp->sb_blocklog)) >ULONG_MAX )) +#else /* Limited by UINT_MAX of sectors */ +if (unlikely ( + (nrblocks << (sbp->sb_blocklog - BBSHIFT)) > UINT_MAX)) +#endif +return XFS_ERROR (E2BIG ); +/* * Calculate new parameters. These are the final values to be reached. */ nrextents = nrblocks;
Quoted 9 lines · Nathan Scott
On the mount path we check for a superblock that describes a filesystem to large for the running kernel to handle. This catches the case of an attempt to mount a >16TB filesystem on i386 (where we are limited by the
page->indexsize, for XFS metadata buffers inxfs_buf.c).This patch makes similar checks on the growfs code paths for regular and realtime growth, else we can end up with filesystem corruption, it would seem (from #xfs chatter). Untested patch follows; probably better to do this as a macro, in a header, and call that in each place...?
Yeah, the check should probably we in one place only. Given that's it's only used in slow pathes a function would probably do it.
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Nathan Scott
On the mount path we check for a superblock that describes a filesystem to large for the running kernel to handle. This catches the case of an attempt to mount a >16TB filesystem on i386 (where we are limited by the
page->indexsize, for XFS metadata buffers inxfs_buf.c).
Quoted 7 lines
Quoted 4 lines
This patch makes similar checks on the growfs code paths for regular and realtime growth, else we can end up with filesystem corruption, it would seem (from #xfs chatter). Untested patch follows; probably better to do this as a macro, in a header, and call that in each place...?
Yeah, the check should probably we in one place only. Given that's it's only used in slow pathes a function would probably do it.
Here's a revised version...
cheers.
Signature · 1 line
Nathan
fs/xfs/xfs_fsops.c +2 -0
@@ -140,6 +140,8 @@ xfs_growfs_data_private( pct = in->imaxpct;if (nb < mp->m_sb.sb_dblocks || pct <0 || pct >100 )return XFS_ERROR (EINVAL ); +if ((error =xfs_sb_validate_fsb_count (&mp->m_sb, nb))) +return error; dpct = pct - mp->m_sb.sb_imax_pct; error = xfs_read_buf(mp, mp->m_ddev_targp, XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1),
fs/xfs/xfs_rtalloc.c +3 -1
@@ -1882,11 +1882,13 @@ xfs_growfs_rt( (nrblocks = in->newblocks) <= sbp->sb_rblocks || (sbp->sb_rblocks && (in->extsize != sbp->sb_rextsize)))return XFS_ERROR (EINVAL ); +if ((error =xfs_sb_validate_fsb_count (sbp, nrblocks))) +return error;/* * Read in the last block of the device, make sure it exists. */ error =xfs_read_buf (mp, mp->m_rtdev_targp, -XFS_FSB_TO_BB (mp, in->newblocks -1 ), +XFS_FSB_TO_BB (mp, nrblocks -1 ),XFS_FSB_TO_BB (mp,1 ),0 , &bp);if (error)return error;
fs/xfs/xfs_mount.c +23 -12
@@ -202,6 +202,27 @@ xfs_mount_free(kmem_free (mp,sizeof (xfs_mount_t)); } +/* +* Check size of device based on the (data/realtime) block count. +* Note: this check is used by the growfs code as well as mount. +*/ +int +xfs_sb_validate_fsb_count ( +xfs_sb_t *sbp, +__uint64_t nblocks) +{ +ASSERT (PAGE_SHIFT >= sbp->sb_blocklog); +ASSERT (sbp->sb_blocklog >=BBSHIFT ); + +#if XFS_BIG_BLKNOS /* Limited by ULONG_MAX of page cache index */ +if (nblocks >> (PAGE_CACHE_SHIFT - sbp->sb_blocklog) >ULONG_MAX ) +return E2BIG ; +#else /* Limited by UINT_MAX of sectors */ +if (nblocks << (sbp->sb_blocklog -BBSHIFT ) >UINT_MAX ) +return E2BIG ; +#endif +return 0 ; +}/* * Check the validity of the SB found.
@@ -284,18 +305,8 @@ xfs_mount_validate_sb(return XFS_ERROR (EFSCORRUPTED ); } -ASSERT (PAGE_SHIFT >= sbp->sb_blocklog); -ASSERT (sbp->sb_blocklog >=BBSHIFT ); - -#if XFS_BIG_BLKNOS /* Limited by ULONG_MAX of page cache index */ -if (unlikely ( - (sbp->sb_dblocks >> (PAGE_SHIFT - sbp->sb_blocklog)) >ULONG_MAX || - (sbp->sb_rblocks >> (PAGE_SHIFT - sbp->sb_blocklog)) >ULONG_MAX )) { -#else /* Limited by UINT_MAX of sectors */ -if (unlikely ( - (sbp->sb_dblocks << (sbp->sb_blocklog -BBSHIFT )) >UINT_MAX || - (sbp->sb_rblocks << (sbp->sb_blocklog -BBSHIFT )) >UINT_MAX )) { -#endif +if (xfs_sb_validate_fsb_count (sbp, sbp->sb_dblocks) || +xfs_sb_validate_fsb_count (sbp, sbp->sb_rblocks)) {xfs_fs_mount_cmn_err (flags,"file system too large to be mounted on this system." );return XFS_ERROR (E2BIG );
fs/xfs/xfs_mount.h +1 -0
@@ -624,6 +624,7 @@ extern int xfs_sync_inodes(xfs_mount_t *extern xfs_agnumber_t xfs_initialize_perag (struct bhv_vfs *,xfs_mount_t *,xfs_agnumber_t );extern void xfs_xlatesb (void *,struct xfs_sb *,int ,__int64_t ); +extern int xfs_sb_validate_fsb_count (struct xfs_sb *,__uint64_t );extern struct xfs_dmops xfs_dmcore_stub;extern struct xfs_qmops xfs_qmcore_stub;
Quoted 11 lines · Nathan Scott
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Nathan Scott
On the mount path we check for a superblock that describes a filesystem to large for the running kernel to handle. This catches the case of an attempt to mount a >16TB filesystem on i386 (where we are limited by the
page->indexsize, for XFS metadata buffers inxfs_buf.c).This patch makes similar checks on the growfs code paths for regular and realtime growth, else we can end up with filesystem corruption, it would seem (from #xfs chatter). Untested patch follows; probably better to do this as a macro, in a header, and call that in each place...?
Quoted 7 lines
Quoted 2 lines
Yeah, the check should probably we in one place only. Given that's it's only used in slow pathes a function would probably do it.
Here's a revised version...
cheers.
Looks good to me
-Eric
Quoted 12 lines · Nathan Scott
Quoted 11 lines · Christoph Hellwig
Quoted 9 lines · Nathan Scott
On the mount path we check for a superblock that describes a filesystem to large for the running kernel to handle. This catches the case of an attempt to mount a >16TB filesystem on i386 (where we are limited by the
page->indexsize, for XFS metadata buffers inxfs_buf.c).This patch makes similar checks on the growfs code paths for regular and realtime growth, else we can end up with filesystem corruption, it would seem (from #xfs chatter). Untested patch follows; probably better to do this as a macro, in a header, and call that in each place...?
Quoted 4 lines
Quoted 2 lines
Yeah, the check should probably we in one place only. Given that's it's only used in slow pathes a function would probably do it.
Here's a revised version...
Added to my qa tree.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group