[PATCH] xfs: prevent 32bit overflow in space reservation

[PATCH] xfs: prevent 32bit overflow in space reservation

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

If we attempt to preallocate more than 2^32 blocks of space in a
single syscall, the transaction block reservation will overflow
leading to a hangs in the superblock block accounting code. This
is trivially reproduced with xfs_io. Fix the problem by capping the
allocation reservation to the maximum number of blocks a single
xfs_bmapi() call can allocate (2^21 blocks).

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_vnodeops.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 66d585c..91dd9c8 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -2299,15 +2299,21 @@ xfs_alloc_file_space(
 			e = allocatesize_fsb;
 		}
 
+		/*
+		 * we can't allocate more than @nimaps extents at a time,
+		 * so prevent a 32bit overflow on the transaction reserve
+		 * by trying to reserve > 16TB worth of blocks for the
+		 * preallocation.
+		 */
+		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));
 		if (unlikely(rt)) {
-			resrtextents = qblocks = (uint)(e - s);
+			resrtextents = qblocks = resblks;
 			resrtextents /= mp->m_sb.sb_rextsize;
 			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 			quota_flag = XFS_QMOPT_RES_RTBLKS;
 		} else {
 			resrtextents = 0;
-			resblks = qblocks = \
-				XFS_DIOSTRAT_SPACE_RES(mp, (uint)(e - s));
+			resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
 			quota_flag = XFS_QMOPT_RES_REGBLKS;
 		}
 
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: prevent 32bit overflow in space reservation

[Christoph Hellwig]

On Thu, Sep 02, 2010 at 03:17:43PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If we attempt to preallocate more than 2^32 blocks of space in a
> single syscall, the transaction block reservation will overflow
> leading to a hangs in the superblock block accounting code. This
> is trivially reproduced with xfs_io. Fix the problem by capping the
> allocation reservation to the maximum number of blocks a single
> xfs_bmapi() call can allocate (2^21 blocks).
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: prevent 32bit overflow in space reservation

[Alex Elder]

On Thu, 2010-09-02 at 15:17 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If we attempt to preallocate more than 2^32 blocks of space in a
> single syscall, the transaction block reservation will overflow
> leading to a hangs in the superblock block accounting code. This
> is trivially reproduced with xfs_io. Fix the problem by capping the
> allocation reservation to the maximum number of blocks a single
> xfs_bmapi() call can allocate (2^21 blocks).

This looks OK, but I have two comments, below.

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_vnodeops.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 66d585c..91dd9c8 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -2299,15 +2299,21 @@ xfs_alloc_file_space(
>  			e = allocatesize_fsb;
>  		}
>  
> +		/*
> +		 * we can't allocate more than @nimaps extents at a time,
> +		 * so prevent a 32bit overflow on the transaction reserve
> +		 * by trying to reserve > 16TB worth of blocks for the
> +		 * preallocation.
> +

This comment could use rewording.  How about something like:

	A 32-bit block count limits the amount of space that can
	be reserved in a transaction, so we need to limit the
	number of blocks reserved to avoid overflow.  We can't
	allocate more than @nimaps extents (whose size won't
	exceed 32 bits) at a time anyway, so use that to enforce
	the limit.

> 		 */
> +		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));

I guess it's clear that MAXEXTLEN fits in 32 bits because of
sizeof (xfs_extlen_t).  And inspection shows that nimaps is
just 1, so this does the 32-bit limiting.  But that just
seems indirect.  (Actually, now that I've written this I
updated the above comment and it's better...)

						-Alex


>  		if (unlikely(rt)) {
> -			resrtextents = qblocks = (uint)(e - s);
> +			resrtextents = qblocks = resblks;
>  			resrtextents /= mp->m_sb.sb_rextsize;
>  			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
>  			quota_flag = XFS_QMOPT_RES_RTBLKS;
>  		} else {
>  			resrtextents = 0;
> -			resblks = qblocks = \
> -				XFS_DIOSTRAT_SPACE_RES(mp, (uint)(e - s));
> +			resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resblks);
>  			quota_flag = XFS_QMOPT_RES_REGBLKS;
>  		}
>  



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: prevent 32bit overflow in space reservation

[Dave Chinner]

On Thu, Sep 02, 2010 at 10:51:19AM -0500, Alex Elder wrote:
> On Thu, 2010-09-02 at 15:17 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > If we attempt to preallocate more than 2^32 blocks of space in a
> > single syscall, the transaction block reservation will overflow
> > leading to a hangs in the superblock block accounting code. This
> > is trivially reproduced with xfs_io. Fix the problem by capping the
> > allocation reservation to the maximum number of blocks a single
> > xfs_bmapi() call can allocate (2^21 blocks).
> 
> This looks OK, but I have two comments, below.
> 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_vnodeops.c |   12 +++++++++---
> >  1 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> > index 66d585c..91dd9c8 100644
> > --- a/fs/xfs/xfs_vnodeops.c
> > +++ b/fs/xfs/xfs_vnodeops.c
> > @@ -2299,15 +2299,21 @@ xfs_alloc_file_space(
> >  			e = allocatesize_fsb;
> >  		}
> >  
> > +		/*
> > +		 * we can't allocate more than @nimaps extents at a time,
> > +		 * so prevent a 32bit overflow on the transaction reserve
> > +		 * by trying to reserve > 16TB worth of blocks for the
> > +		 * preallocation.
> > +
> 
> This comment could use rewording.  How about something like:
> 
> 	A 32-bit block count limits the amount of space that can
> 	be reserved in a transaction, so we need to limit the
> 	number of blocks reserved to avoid overflow.  We can't
> 	allocate more than @nimaps extents (whose size won't
> 	exceed 32 bits) at a time anyway, so use that to enforce
> 	the limit.

Ok, make sense - I'll reword it.

> > 		 */
> > +		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));
> 
> I guess it's clear that MAXEXTLEN fits in 32 bits because of
> sizeof (xfs_extlen_t).

True, but if sizeof(xfs_extlen_t) was the limiting factor,
then the mulitply could still cause 32bit overflows.

The real reason is that MAXEXTLEN defines the maximum extent length
supported by the on disk bmap btree record format. The record format
defines the extent length in FSBs to be:

#define MAXEXTLEN       ((xfs_extlen_t)0x001fffff)      /* 21 bits */

and as such fits easily into the 32 bit limit.

> And inspection shows that nimaps is
> just 1, so this does the 32-bit limiting.  But that just
> seems indirect.

nimaps can be up to:

#define XFS_BMAP_MAX_NMAP 4

So if we change the loop to do more allocations per loop, then
the code will already handle it correctly. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: prevent 32bit overflow in space reservation

[Alex Elder]

On Fri, 2010-09-03 at 10:01 +1000, Dave Chinner wrote:
> On Thu, Sep 02, 2010 at 10:51:19AM -0500, Alex Elder wrote:
> > On Thu, 2010-09-02 at 15:17 +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > If we attempt to preallocate more than 2^32 blocks of space in a
. . .

> > > +		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));
> > 
> > I guess it's clear that MAXEXTLEN fits in 32 bits because of
> > sizeof (xfs_extlen_t).
> 
> True, but if sizeof(xfs_extlen_t) was the limiting factor,
> then the mulitply could still cause 32bit overflows.
> 
> The real reason is that MAXEXTLEN defines the maximum extent length
> supported by the on disk bmap btree record format. The record format
> defines the extent length in FSBs to be:
> 
> #define MAXEXTLEN       ((xfs_extlen_t)0x001fffff)      /* 21 bits */
> 
> and as such fits easily into the 32 bit limit.

Yes, I recognized that but didn't mention it.  However...

> > And inspection shows that nimaps is
> > just 1, so this does the 32-bit limiting.  But that just
> > seems indirect.
> 
> nimaps can be up to:
> 
> #define XFS_BMAP_MAX_NMAP 4

...I had not noticed that nimap could have been changed from
its value 1 by the xfs_bmapi() call, so the point you make is
important.

> So if we change the loop to do more allocations per loop, then
> the code will already handle it correctly. :)

Yes.  And like I said, just adjusting the comment explains
why it is safe.

					-Alex


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs