* [PATCH] make growfs check device size limits too @ 2007-04-26 6:30 Nathan Scott 2007-04-26 7:10 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Nathan Scott @ 2007-04-26 6:30 UTC (permalink / raw) To: xfs [-- Attachment #1: Type: text/plain, Size: 578 bytes --] 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. -- Nathan [-- Attachment #2: growfs.patch --] [-- Type: text/x-patch, Size: 1759 bytes --] --- fs/xfs/xfs_fsops.c.orig 2007-04-26 16:05:38.126936000 +1000 +++ fs/xfs/xfs_fsops.c 2007-04-26 16:17:03.385762000 +1000 @@ -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); --- fs/xfs/xfs_rtalloc.c.orig 2007-04-26 16:16:34.695969000 +1000 +++ fs/xfs/xfs_rtalloc.c 2007-04-26 16:22:43.227000750 +1000 @@ -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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make growfs check device size limits too 2007-04-26 6:30 [PATCH] make growfs check device size limits too Nathan Scott @ 2007-04-26 7:10 ` Christoph Hellwig 2007-04-26 23:45 ` Nathan Scott 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2007-04-26 7:10 UTC (permalink / raw) To: Nathan Scott; +Cc: xfs On Thu, Apr 26, 2007 at 04:30:14PM +1000, Nathan Scott wrote: > 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...? 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make growfs check device size limits too 2007-04-26 7:10 ` Christoph Hellwig @ 2007-04-26 23:45 ` Nathan Scott 2007-04-27 2:24 ` Eric Sandeen 2007-04-27 6:16 ` David Chinner 0 siblings, 2 replies; 5+ messages in thread From: Nathan Scott @ 2007-04-26 23:45 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs [-- Attachment #1: Type: text/plain, Size: 905 bytes --] On Thu, 2007-04-26 at 08:10 +0100, Christoph Hellwig wrote: > On Thu, Apr 26, 2007 at 04:30:14PM +1000, Nathan Scott wrote: > > 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...? > > 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. -- Nathan [-- Attachment #2: fix-sb-size-checks --] [-- Type: text/x-patch, Size: 3816 bytes --] Index: linux/fs/xfs/xfs_fsops.c =================================================================== --- linux.orig/fs/xfs/xfs_fsops.c 2007-04-27 09:00:57.306146750 +1000 +++ linux/fs/xfs/xfs_fsops.c 2007-04-27 09:41:22.897736750 +1000 @@ -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), Index: linux/fs/xfs/xfs_rtalloc.c =================================================================== --- linux.orig/fs/xfs/xfs_rtalloc.c 2007-04-27 09:16:57.558158750 +1000 +++ linux/fs/xfs/xfs_rtalloc.c 2007-04-27 09:38:03.705288000 +1000 @@ -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; Index: linux/fs/xfs/xfs_mount.c =================================================================== --- linux.orig/fs/xfs/xfs_mount.c 2007-04-27 09:00:57.354149750 +1000 +++ linux/fs/xfs/xfs_mount.c 2007-04-27 09:42:07.700536750 +1000 @@ -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); Index: linux/fs/xfs/xfs_mount.h =================================================================== --- linux.orig/fs/xfs/xfs_mount.h 2007-04-27 09:25:44.667101000 +1000 +++ linux/fs/xfs/xfs_mount.h 2007-04-27 09:37:43.448022000 +1000 @@ -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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make growfs check device size limits too 2007-04-26 23:45 ` Nathan Scott @ 2007-04-27 2:24 ` Eric Sandeen 2007-04-27 6:16 ` David Chinner 1 sibling, 0 replies; 5+ messages in thread From: Eric Sandeen @ 2007-04-27 2:24 UTC (permalink / raw) To: nscott; +Cc: Christoph Hellwig, xfs Nathan Scott wrote: > On Thu, 2007-04-26 at 08:10 +0100, Christoph Hellwig wrote: >> On Thu, Apr 26, 2007 at 04:30:14PM +1000, Nathan Scott wrote: >>> 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...? >> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make growfs check device size limits too 2007-04-26 23:45 ` Nathan Scott 2007-04-27 2:24 ` Eric Sandeen @ 2007-04-27 6:16 ` David Chinner 1 sibling, 0 replies; 5+ messages in thread From: David Chinner @ 2007-04-27 6:16 UTC (permalink / raw) To: Nathan Scott; +Cc: Christoph Hellwig, xfs On Fri, Apr 27, 2007 at 09:45:20AM +1000, Nathan Scott wrote: > On Thu, 2007-04-26 at 08:10 +0100, Christoph Hellwig wrote: > > On Thu, Apr 26, 2007 at 04:30:14PM +1000, Nathan Scott wrote: > > > 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...? > > > > 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. -- Dave Chinner Principal Engineer SGI Australian Software Group ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-04-27 6:16 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-04-26 6:30 [PATCH] make growfs check device size limits too Nathan Scott 2007-04-26 7:10 ` Christoph Hellwig 2007-04-26 23:45 ` Nathan Scott 2007-04-27 2:24 ` Eric Sandeen 2007-04-27 6:16 ` David Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox