[PATCH] make growfs check device size limits too

[PATCH] make growfs check device size limits too

[Nathan Scott]

[-- 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;

Re: [PATCH] make growfs check device size limits too

[Christoph Hellwig]

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.

Re: [PATCH] make growfs check device size limits too

[Nathan Scott]

[-- 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;

Re: [PATCH] make growfs check device size limits too

[Eric Sandeen]

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

Re: [PATCH] make growfs check device size limits too

[David Chinner]

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