Prevent creation of files larger than RLIMIT_FSIZE using fallocate

PATCH In review Nikanth Karthikesan 9 participants 19 messages Apr 28, 2010 – May 21, 2010 2 messages not in the archive

Subsystems (3)

VFS Btrfs file system ext4 file system

lore

Share & tools

lore

Nikanth Karthikesan

author

2010-04-28 13:23 UTC #0 lore raw

PATCH 1/1 Prevent creation of files larger than RLIMIT_FSIZE using fallocate

Prevent creation of files larger than RLIMIT_FSIZE using fallocate.

Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit and create a file larger than the limit. Add a check for new size in the fallocate system call.

File-systems supporting fallocate such as ext4 are affected by this bug.

Signed-off-by Nikanth Karthikesan <[email protected]>

Reported-by Eelis - <[email protected]>

---

fs/open.c +6 -2

index 74e5cd9..95ce069 100644
@@ -412,10 +412,14 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 		return -ENODEV;
 
-	/* Check for wrap through zero too */
-	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
+	/* Check for wrap through zero */
+	if (offset+len < 0)
 		return -EFBIG;
 
+	ret = inode_newsize_ok(inode, (offset + len));
+	if (ret)
+		return ret;
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;

Coly Li

2010-04-28 16:02 UTC #1 lore raw

On 04/28/2010 09:24 PM, Nikanth Karthikesan Wrote:

Quoted 25 lines

Prevent creation of files larger than RLIMIT_FSIZE using fallocate.
Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit and create a file larger than the limit. Add a check for new size in the fallocate system call.
File-systems supporting fallocate such as ext4 are affected by this bug.
Signed-off-by Nikanth Karthikesan <[email protected]>
Reported-by Eelis - <[email protected]>
---
fs/open.c +1 -2
index 74e5cd9..95ce069 100644
@@ -412,10 +412,14 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 		return -ENODEV;
 
-	/* Check for wrap through zero too */
-	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
+	/* Check for wrap through zero */

fs/open.c int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) · line 425

Quoted 10 lines

fs/open.c +5 -0

+	if (offset+len < 0)
 		return -EFBIG;
 
+	ret = inode_newsize_ok(inode, (offset + len));
+	if (ret)
+		return ret;
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;

Hi Nikanth,

Quoted 1 line

From definition of inode_newsize_ok(), it says,

64 /**

 65  * inode_newsize_ok - may this inode be truncated to a given size
 66  * @inode:      the inode to be truncated
 67  * @offset:     the new size to assign to the inode
 68  * @Returns:    0 on success, -ve errno on failure
 69  *
 70  * inode_newsize_ok will check filesystem limits and ulimits to check that the
 71  * new inode size is within limits. inode_newsize_ok will also send SIGXFSZ
 72  * when necessary. Caller must not proceed with inode size change if failure is
 73  * returned. @inode must be a file (not directory), with appropriate
 74  * permissions to allow truncate (inode_newsize_ok does NOT check these
 75  * conditions).
 76  *
 77  * inode_newsize_ok must be called with i_mutex held.
 78  */

In execution path of do_fallocate(), it seems no i_mutex held, and inode might be directory. Using inode_newsize_ok() might be confused ?

How about this one ?

 fs/open.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

fs/open.c +12 -2

index 74e5cd9..24d75a4 100644
@@ -383,6 +383,7 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftruncate64);
 int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
+	unsigned long limit;
 	long ret;
 
 	if (offset < 0 || len <= 0)

@@ -412,10 +413,19 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 		return -ENODEV;
 
-	/* Check for wrap through zero too */
-	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
+	/* Check for wrap through zero */
+	if ((offset + len) < 0)
 		return -EFBIG;
 
+	/* Check for rlimit */
+	if ((offset + len) > inode->i_size) {
+		limit = rlimit(RLIMIT_FSIZE);
+		if (limit != RLIM_INFINITY && (offset + len) > limit)
+			return -EFBIG;
+		if ((offset + len) > inode->i_sb->s_maxbytes)
+			return -EFBIG;
+	}
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;

Something I don't understand here is, why no i_mutex hold here ?

Signature · 2 lines

Coly Li
SuSE Labs

Andreas Dilger

2010-04-29 09:23 UTC #4 lore raw

In reply to a message not in the archive

Quoted 8 lines · Coly Li

Review
Quoted 6 lines
(quoted diff) +6 -0
+		/*
+		 * Let individual file system decide if it supports
+		 * preallocation for directories or not.
+		 */
+		if (offset > inode->i_sb->s_maxbytes)
+			return -EFBIG;
For file systems support online-resize, like ext4 and ocfs2, should we also need a locking to protect inode->i_sb->s_mabytes ?

s_maxbytes is the maximum theoretical single file size, and has nothing to do with the size of the filesystem.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Andrew Morton

2010-04-30 21:33 UTC #5 lore raw

In reply to a message not in the archive

(Amit Arora <[email protected]> wrote fallocate. cc added)

Quoted 2 lines · On Thu, 29 Apr 2010 10:14:06 +0530 Nikanth Karthikesan <[email protected]> wrote:

Here is an updated patch that takes the i_mutex and calls inode_newsize_ok() only for regular files.

err, no. It's taking i_lock where it meant to take i_mutex.

Quoted 27 lines

Thanks
Nikanth
Prevent creation of files larger than RLIMIT_FSIZE using fallocate.
Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit and create a file larger than the limit. Add a check for new size in the fallocate system call.
File-systems supporting fallocate such as ext4 are affected by this bug.
Signed-off-by Nikanth Karthikesan <[email protected]>
Reported-by Eelis - <[email protected]>
---
fs/open.c +0 -2
index 74e5cd9..4ca57c9 100644
@@ -405,17 +405,26 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (S_ISFIFO(inode->i_mode))
 		return -ESPIPE;
 
-	/*
-	 * Let individual file system decide if it supports preallocation

fs/open.c int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) · line 410

Quoted 10 lines

fs/open.c +2 -7

-	 * for directories or not.
-	 */
-	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		return -ENODEV;
-
-	/* Check for wrap through zero too */
-	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
+	/* Check for wrap through zero */
+	if (offset+len < 0)
 		return -EFBIG;

I suggest that this test be moved up to where the function tests `if (offset < 0 || len <= 0)' - it seems more logical.

Also,

- if (offset+len < 0)
+ if (offset + len < 0)

for consistency with most other kernel code, please.

Quoted 8 lines

fs/open.c +8 -0

+	if (S_ISREG(inode->i_mode)) {
+		spin_lock(&inode->i_lock);
+		ret = inode_newsize_ok(inode, (offset + len));
+		spin_unlock(&inode->i_lock);
+		if (ret)
+			return ret;
+	} else if (S_ISDIR(inode->i_mode)) {
+		/*

fs/open.c int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) · line 428

Quoted 10 lines

fs/open.c +8 -0

+		 * Let individual file system decide if it supports
+		 * preallocation for directories or not.
+		 */
+		if (offset > inode->i_sb->s_maxbytes)
+			return -EFBIG;
+	} else
+		return -ENODEV;
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;

Also, there doesn't seem to be much point in doing

	mutex_lock(i_mutex);
	if (some_condition)
		bale out
	mutex_unlock(i_mutex);

	<stuff>

because some_condition' can now become true before or during the execution of stuff'.

IOW, it's racy.

Andreas Dilger

2010-04-30 21:40 UTC #6 lore raw

Quoted 10 lines · Andrew Morton

Quoted 8 lines · On Thu, 29 Apr 2010 10:14:06 +0530 Nikanth Karthikesan <[email protected]> wrote:
fs/open.c +0 -0
index 74e5cd9..4ca57c9 100644
@@ -405,17 +405,26 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (S_ISFIFO(inode->i_mode))
 		return -ESPIPE;
 

Quoted 15 lines

fs/open.c int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len) · line 410
Quoted 12 lines
fs/open.c +2 -9
-	/*
-	 * Let individual file system decide if it supports preallocation
-	 * for directories or not.
-	 */
-	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		return -ENODEV;
-
-	/* Check for wrap through zero too */
-	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
+	/* Check for wrap through zero */
+	if (offset+len < 0)
 		return -EFBIG;
I suggest that this test be moved up to where the function tests `if (offset < 0 || len <= 0)' - it seems more logical.

Sometimes the order of these checks is mandated by POSIX because of the error return code. I'm not saying for sure that is the case here, but sometimes logic doesn't come into the specification. :-/

Cheers, Andreas
--
Andreas Dilger
Lustre Technical Lead
Oracle Corporation Canada Inc.

Amit K. Arora

2010-05-01 07:04 UTC #7 lore raw

In reply to Andrew Morton

Quoted 2 lines · Andrew Morton

(Amit Arora <[email protected]> wrote fallocate. cc added)

Thanks for adding me to CC.

Quoted 39 lines

Quoted 2 lines · On Thu, 29 Apr 2010 10:14:06 +0530 Nikanth Karthikesan <[email protected]> wrote:
Here is an updated patch that takes the i_mutex and calls inode_newsize_ok() only for regular files.
err, no. It's taking i_lock where it meant to take i_mutex.
Quoted 11 lines
Thanks
Nikanth
(quoted diff) +8 -0
+	if (S_ISREG(inode->i_mode)) {
+		spin_lock(&inode->i_lock);
+		ret = inode_newsize_ok(inode, (offset + len));
+		spin_unlock(&inode->i_lock);
+		if (ret)
+			return ret;
+	} else if (S_ISDIR(inode->i_mode)) {
+		/*
Review
Quoted 10 lines
(quoted diff) +8 -0
+		 * Let individual file system decide if it supports
+		 * preallocation for directories or not.
+		 */
+		if (offset > inode->i_sb->s_maxbytes)
+			return -EFBIG;
+	} else
+		return -ENODEV;
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;
Also, there doesn't seem to be much point in doing
	mutex_lock(i_mutex);
	if (some_condition)
		bale out
	mutex_unlock(i_mutex);

	<stuff>

Quoted 4 lines

because some_condition' can now become true before or during the execution of stuff'.
IOW, it's racy.

Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :

fs/ext4/extents.c +5 -0

diff -Nuarp linux-2.6.org/fs/ext4/extents.c linux-2.6.new/fs/ext4/extents.c
@@ -3672,6 +3672,11 @@ long ext4_fallocate(struct inode *inode,
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
 	mutex_lock(&inode->i_mutex);
+	ret = inode_newsize_ok(inode, (offset + len));
+	if (ret) {
+		mutex_unlock(&inode->i_mutex);
+		return ret;
+	}
 retry:
 	while (ret >= 0 && ret < max_blocks) {
 		block = block + ret;

Similarly for ocfs2, btrfs and xfs..

--
Regards,
Amit Arora

Christoph Hellwig

2010-05-01 10:18 UTC #8 lore raw

Quoted 2 lines · Amit K. Arora

Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :

That looks okay - in fact XFS should already have this check because it re-uses the setattr implementation to set the size.

Can you submit an xfstests testcase to verify this behaviour on all filesystems?

Nikanth Karthikesan

author

2010-05-03 04:22 UTC #14 lore raw

In reply to Amit K. Arora

Quoted 49 lines · On Saturday 01 May 2010 12:34:26 Amit K. Arora wrote:

Quoted 1 line · Andrew Morton
(Amit Arora <[email protected]> wrote fallocate. cc added)
Thanks for adding me to CC.
Quoted 43 lines
On Thu, 29 Apr 2010 10:14:06 +0530
Quoted 2 lines · Nikanth Karthikesan
Here is an updated patch that takes the i_mutex and calls inode_newsize_ok() only for regular files.
err, no. It's taking i_lock where it meant to take i_mutex.
Quoted 11 lines
Thanks
Nikanth
(quoted diff) +8 -0
+	if (S_ISREG(inode->i_mode)) {
+		spin_lock(&inode->i_lock);
+		ret = inode_newsize_ok(inode, (offset + len));
+		spin_unlock(&inode->i_lock);
+		if (ret)
+			return ret;
+	} else if (S_ISDIR(inode->i_mode)) {
+		/*
Review
Quoted 10 lines
(quoted diff) +8 -0
+		 * Let individual file system decide if it supports
+		 * preallocation for directories or not.
+		 */
+		if (offset > inode->i_sb->s_maxbytes)
+			return -EFBIG;
+	} else
+		return -ENODEV;
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;
Also, there doesn't seem to be much point in doing
	mutex_lock(i_mutex);
	if (some_condition)
		bale out
	mutex_unlock(i_mutex);

	<stuff>
because some_condition' can now become true before or during the execution of stuff'.
IOW, it's racy.

oh, yes. :(

Quoted 3 lines

Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :

I guess, calling the filesystem specific fallocate with the lock held would create lock ordering problems? If so, this might be the only way. But it would be better to document at the call site, that the callee should check for RLIMIT_FSIZE.

Thanks
Nikanth

Quoted 23 lines

diff -Nuarp linux-2.6.org/fs/ext4/extents.c linux-2.6.new/fs/ext4/extents.c

fs/ext4/extents.c +5 -0

@@ -3672,6 +3672,11 @@ long ext4_fallocate(struct inode *inode,
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
 	mutex_lock(&inode->i_mutex);
+	ret = inode_newsize_ok(inode, (offset + len));
+	if (ret) {
+		mutex_unlock(&inode->i_mutex);
+		return ret;
+	}
 retry:
 	while (ret >= 0 && ret < max_blocks) {
 		block = block + ret;

Similarly for ocfs2, btrfs and xfs..

--
Regards,
Amit Arora

Amit K. Arora

2010-05-03 06:59 UTC #15 lore raw

Quoted 18 lines · Nikanth Karthikesan

Quoted 8 lines · On Saturday 01 May 2010 12:34:26 Amit K. Arora wrote:
Quoted 7 lines · Andrew Morton
Also, there doesn't seem to be much point in doing
	mutex_lock(i_mutex);
	if (some_condition)
		bale out
	mutex_unlock(i_mutex);
Quoted 6 lines
Quoted 6 lines
	<stuff>
because some_condition' can now become true before or during the execution of stuff'.
IOW, it's racy.
oh, yes. :(

Quoted 7 lines

Quoted 2 lines
Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :
I guess, calling the filesystem specific fallocate with the lock held would create lock ordering problems? If so, this might be the only way. But it would be better to document at the call site, that the callee should check for RLIMIT_FSIZE.

Hmm.. I never said to call the filesystem specific fallocate with i_mutex held. What I suggested was that each filesystem at some point anyhow takes the i_mutex to preallocate. Thats where the check should be, to avoid the race. This is what the example patch below does.

--
Regards,
Amit Arora

Quoted 26 lines

Thanks
Nikanth

Quoted 23 lines

diff -Nuarp linux-2.6.org/fs/ext4/extents.c linux-2.6.new/fs/ext4/extents.c

fs/ext4/extents.c +5 -0

@@ -3672,6 +3672,11 @@ long ext4_fallocate(struct inode *inode,
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
 	mutex_lock(&inode->i_mutex);
+	ret = inode_newsize_ok(inode, (offset + len));
+	if (ret) {
+		mutex_unlock(&inode->i_mutex);
+		return ret;
+	}
 retry:
 	while (ret >= 0 && ret < max_blocks) {
 		block = block + ret;

Similarly for ocfs2, btrfs and xfs..

--
Regards,
Amit Arora

Amit K. Arora

2010-05-03 07:01 UTC #9 lore raw

In reply to Christoph Hellwig

Quoted 6 lines · Christoph Hellwig

Quoted 2 lines · Amit K. Arora
Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :
That looks okay - in fact XFS should already have this check because it re-uses the setattr implementation to set the size.

You are right. I have written a test and it passes on XFS, but fails on ext4.

Quoted 2 lines

Can you submit an xfstests testcase to verify this behaviour on all filesystems?

Sure. Its ready and is under test. Will post shortly..

--
Regards,
Amit Arora

Nikanth Karthikesan

author

2010-05-03 07:48 UTC #16 lore raw

In reply to Amit K. Arora

Quoted 27 lines · On Monday 03 May 2010 12:29:45 Amit K. Arora wrote:

Quoted 18 lines · Nikanth Karthikesan
Quoted 8 lines · On Saturday 01 May 2010 12:34:26 Amit K. Arora wrote:
Quoted 7 lines · Andrew Morton
Also, there doesn't seem to be much point in doing
	mutex_lock(i_mutex);
	if (some_condition)
		bale out
	mutex_unlock(i_mutex);
Quoted 6 lines
Quoted 6 lines
	<stuff>
because some_condition' can now become true before or during the execution of stuff'.
IOW, it's racy.
oh, yes. :(
Quoted 7 lines
Quoted 2 lines
Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :
I guess, calling the filesystem specific fallocate with the lock held would create lock ordering problems? If so, this might be the only way. But it would be better to document at the call site, that the callee should check for RLIMIT_FSIZE.

Quoted 5 lines

Hmm.. I never said to call the filesystem specific fallocate with i_mutex held. What I suggested was that each filesystem at some point anyhow takes the i_mutex to preallocate. Thats where the check should be, to avoid the race. This is what the example patch below does.

Yes, you never said that. But I just wondered whether that would be have problems and doing the check in filesystem specific fallocate is the only solution. :)

Thanks
Nikanth

Amit K. Arora

2010-05-03 08:29 UTC #10 lore raw

PATCH 1/1 New testcase to check if fallocate respects RLIMIT_FSIZE or not

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

Quoted 2 lines · Amit K. Arora
Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :
That looks okay - in fact XFS should already have this check because it re-uses the setattr implementation to set the size.
Can you submit an xfstests testcase to verify this behaviour on all filesystems?

Here is the new testcase.

I have run this test on a x86_64 box on XFS and ext4 on 2.6.34-rc6. It passes on XFS, but fails on ext4. Below is the snapshot of results followed by the testcase itself.

--
Regards,
Amit Arora

Test results:
------------

# ./check 228
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 elm9m93 2.6.34-rc6

228 0s ...
Ran: 228
Passed all 1 tests
#

# umount /mnt
# mkfs.ext4 /dev/sda4 >/dev/null
mke2fs 1.41.10 (10-Feb-2009)
# ./check 228
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 elm9m93 2.6.34-rc6

228 0s ... - output mismatch (see 228.out.bad)

228.out.bad renamed from 228.out +0 -1

@@ -1,2 +1 @@
 QA output created by 228
-File size limit exceeded (core dumped)

Ran: 228
Failures: 228
Failed 1 of 1 tests
#

Here is the test:
----------------
Add a new testcase to the xfstests suite to check if fallocate respects the limit imposed by RLIMIT_FSIZE (can be set by "ulimit -f XXX") or not, on a particular filesystem.

Signed-off-by Amit Arora <[email protected]>

228 +79 -0

79 lines not on this page · raw message

group +1 -0

diff -Nuarp xfstests-dev.org/group xfstests-dev/group
@@ -341,3 +341,4 @@ deprecated
 225 auto quick
 226 auto enospc
 227 auto fsr
+228 rw auto prealloc quick

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Nikanth Karthikesan

author

2010-05-04 05:43 UTC #17 lore raw

PATCH 1/1 btrfs: Prevent creation of files larger than RLIMIT_FSIZE using fallocate

In reply to Amit K. Arora

Prevent creation of files larger than RLIMIT_FSIZE using fallocate.

Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit and create a file larger than the limit. Add a check for that.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

fs/btrfs/inode.c +5 -0

index 2bfdc64..a1d8fbc 100644
@@ -5829,6 +5829,11 @@ static long btrfs_fallocate(struct inode *inode, int mode,
 	btrfs_wait_ordered_range(inode, alloc_start, alloc_end - alloc_start);
 
 	mutex_lock(&inode->i_mutex);
+
+	ret = inode_newsize_ok(inode, (offset + len));
+	if (ret)
+		goto out;
+
 	if (alloc_start > inode->i_size) {
 		ret = btrfs_cont_expand(inode, alloc_start);
 		if (ret)

Nikanth Karthikesan

author

2010-05-04 05:43 UTC #18 lore raw

PATCH 1/1 ext4: Prevent creation of files larger than RLIMIT_FSIZE using fallocate

In reply to Amit K. Arora

I assumed that Amit would send a patch with s-o-b, if not, please take this patch.

Thanks
Nikanth

Prevent creation of files larger than RLIMIT_FSIZE using fallocate.

Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit and create a file larger than the limit. Add a check for that.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

fs/ext4/extents.c +5 -0

index 236b834..39b8123 100644
@@ -3672,6 +3672,11 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
 	mutex_lock(&inode->i_mutex);
+	ret = inode_newsize_ok(inode, (len + offset));
+	if (ret) {
+		mutex_unlock(&inode->i_mutex);
+		return ret;
+	}
 retry:
 	while (ret >= 0 && ret < max_blocks) {
 		block = block + ret;

Amit K. Arora

2010-05-04 06:28 UTC #19 lore raw

Re: [PATCH] ext4: Prevent creation of files larger than RLIMIT_FSIZE using fallocate

Quoted 2 lines · Nikanth Karthikesan

I assumed that Amit would send a patch with s-o-b, if not, please take this patch.

Have added my sign-off below. Thanks!

--
Regards,
Amit Arora

Quoted 10 lines

Thanks
Nikanth
Prevent creation of files larger than RLIMIT_FSIZE using fallocate.
Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit and create a file larger than the limit. Add a check for that.
Signed-off-by Nikanth Karthikesan <[email protected]>

Signed-off-by Amit Arora <[email protected]>

Quoted 18 lines

---

fs/ext4/extents.c +5 -0

index 236b834..39b8123 100644
@@ -3672,6 +3672,11 @@ long ext4_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)
 	 */
 	credits = ext4_chunk_trans_blocks(inode, max_blocks);
 	mutex_lock(&inode->i_mutex);
+	ret = inode_newsize_ok(inode, (len + offset));
+	if (ret) {
+		mutex_unlock(&inode->i_mutex);
+		return ret;
+	}
 retry:
 	while (ret >= 0 && ret < max_blocks) {
 		block = block + ret;

Eric Sandeen

2010-05-04 20:42 UTC #11 lore raw

Re: [PATCH] New testcase to check if fallocate respects RLIMIT_FSIZE or not

In reply to Amit K. Arora

Quoted 11 lines · Amit K. Arora

Quoted 8 lines · Christoph Hellwig
Quoted 2 lines · Amit K. Arora
Agreed. How about doing this check in the filesystem specific fallocate inode routines instead ? For example, in ext4 we could do :
That looks okay - in fact XFS should already have this check because it re-uses the setattr implementation to set the size.
Can you submit an xfstests testcase to verify this behaviour on all filesystems?
Here is the new testcase.

Thanks! A few comments...

Quoted 31 lines

I have run this test on a x86_64 box on XFS and ext4 on 2.6.34-rc6. It passes on XFS, but fails on ext4. Below is the snapshot of results followed by the testcase itself.
--
Regards,
Amit Arora
Test results:
------------
# ./check 228
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 elm9m93 2.6.34-rc6
228 0s ...
Ran: 228
Passed all 1 tests
#
# umount /mnt
# mkfs.ext4 /dev/sda4 >/dev/null
mke2fs 1.41.10 (10-Feb-2009)
# ./check 228
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 elm9m93 2.6.34-rc6
228 0s ... - output mismatch (see 228.out.bad)
228.out.bad renamed from 228.out +0 -1
@@ -1,2 +1 @@
 QA output created by 228
-File size limit exceeded (core dumped)

Quoted 4 lines

Ran: 228
Failures: 228
Failed 1 of 1 tests
#

228.out is missing from the patch

Also on my fedora box I don't get a coredump by default; can you either make that explicit, or filter out the core message?

Quoted 6 lines

Here is the test:
----------------
Add a new testcase to the xfstests suite to check if fallocate respects the limit imposed by RLIMIT_FSIZE (can be set by "ulimit -f XXX") or not, on a particular filesystem.

...

Quoted 3 lines

+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter

Nitpick, I don't think you need common.filter, doesn't look like you are using it.

228.out.bad

Quoted 8 lines

228.out.bad renamed from 228.out +8 -0

+# FSIZE limit is now set to 100 MB.
+# Lets try to preallocate 101 MB. This should fail.
+$XFS_IO_PROG -F -f -c 'falloc 0 101m' $TEST_DIR/ouch
+rm -f $TEST_DIR/ouch
+
+# Lets now try to preallocate 50 MB. This should succeed.
+$XFS_IO_PROG -F -f -c 'falloc 0 50m' $TEST_DIR/ouch
+rm -f $TEST_DIR/ouch

Even more nitpicky, but sometimes I think it's nice to have the .out file be a bit more descriptive in and of itself so when you see a failing diff you have a better idea what's gone wrong.

Changing the comments to echos, like:

+# FSIZE limit is now set to 100 MB.
+# echo "Lets try to preallocate 101 MB. This should fail." +$XFS_IO_PROG -F -f -c 'falloc 0 101m' $TEST_DIR/ouch +rm -f $TEST_DIR/ouch

etc ... would make a failure look like:

228.out.bad renamed from 228.out +0 -1

@@ -1,3 +1,2 @@
 QA output created by 228
 Lets try to preallocate 101 MB. This should fail.
-File size limit exceeded

Lets now try to preallocate 50 MB. This should succeed.

... just a thought.

Thanks,
-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Amit K. Arora

2010-05-05 07:54 UTC #12 lore raw

PATCH v2 1/1 New testcase to check if fallocate respects RLIMIT_FSIZE or not

Quoted 3 lines · Eric Sandeen

Quoted 1 line · Amit K. Arora
Here is the new testcase.
Thanks! A few comments...

Thanks for the review!

Quoted 1 line

228.out is missing from the patch

Ok, added it in the new patch.

Quoted 2 lines

Also on my fedora box I don't get a coredump by default; can you either make that explicit, or filter out the core message?

Hmm.. for some strange reason I am no longer seeing this message. Tried on the same system as last time and couple of others also.

Quoted 7 lines

Quoted 6 lines
Here is the test:
----------------
Add a new testcase to the xfstests suite to check if fallocate respects the limit imposed by RLIMIT_FSIZE (can be set by "ulimit -f XXX") or not, on a particular filesystem.

Quoted 8 lines

...
Quoted 3 lines
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
Nitpick, I don't think you need common.filter, doesn't look like you are using it.

Right. Removed it..

Quoted 4 lines

(quoted diff) +4 -0

+# FSIZE limit is now set to 100 MB.
+# Lets try to preallocate 101 MB. This should fail.
+$XFS_IO_PROG -F -f -c 'falloc 0 101m' $TEST_DIR/ouch
+rm -f $TEST_DIR/ouch

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +4 -0
+
+# Lets now try to preallocate 50 MB. This should succeed.
+$XFS_IO_PROG -F -f -c 'falloc 0 50m' $TEST_DIR/ouch
+rm -f $TEST_DIR/ouch
Even more nitpicky, but sometimes I think it's nice to have the .out file be a bit more descriptive in and of itself so when you see a failing diff you have a better idea what's gone wrong.

Agreed. Done.

Here is the new patch with the changes:

Add a new testcase to the xfstests suite to check if fallocate respects the limit imposed by RLIMIT_FSIZE (can be set by "ulimit -f XXX") or not, on a particular filesystem.

Signed-off-by Amit Arora <[email protected]>

228 +79 -0

diff -Nuarp xfstests-dev.org/228 xfstests-dev/228
@@ -0,0 +1,79 @@
+#! /bin/bash
+# FS QA Test No. 228
+#
+# Check if fallocate respects RLIMIT_FSIZE 
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2010 IBM Corporation. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+here=`pwd`
+tmp=$TEST_DIR/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15 25
+
+# get standard environment, filters and checks
+. ./common.rc
+
+# real QA test starts here
+# generic, but xfs_io's fallocate must work
+_supported_fs generic
+# only Linux supports fallocate
+_supported_os Linux
+
+[ -n "$XFS_IO_PROG" ] || _notrun "xfs_io executable not found"
+
+rm -f $seq.full
+
+# Sanity check to see if fallocate works
+_require_xfs_io_falloc
+
+# Check if we have good enough space available
+avail=`df -P $TEST_DIR | awk 'END {print $4}'`
+[ "$avail" -ge 104000 ] || _notrun "Test device is too small ($avail KiB)"
+
+# Set the FSIZE ulimit to 100MB and check
+ulimit -f 102400
+flim=`ulimit -f`
+[ "$flim" != "unlimited" ] || _notrun "Unable to set FSIZE ulimit"
+[ "$flim" -eq 102400 ] || _notrun "FSIZE ulimit is not correct (100 MB)"
+
+echo "File size limit is now set to 100 MB."
+echo "Let us try to preallocate 101 MB. This should fail."
+$XFS_IO_PROG -F -f -c 'falloc 0 101m' $TEST_DIR/ouch
+rm -f $TEST_DIR/ouch
+
+echo "Let us now try to preallocate 50 MB. This should succeed."
+$XFS_IO_PROG -F -f -c 'falloc 0 50m' $TEST_DIR/ouch
+rm -f $TEST_DIR/ouch
+
+echo "Test over."
+# success, all done
+status=0
+exit

228.out +6 -0

diff -Nuarp xfstests-dev.org/228.out xfstests-dev/228.out
@@ -0,0 +1,6 @@
+QA output created by 228
+File size limit is now set to 100 MB.
+Let us try to preallocate 101 MB. This should fail.
+File size limit exceeded
+Let us now try to preallocate 50 MB. This should succeed.
+Test over.

group +1 -0

diff -Nuarp xfstests-dev.org/group xfstests-dev/group
@@ -341,3 +341,4 @@ deprecated
 225 auto quick
 226 auto enospc
 227 auto fsr
+228 rw auto prealloc quick

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2010-05-05 15:48 UTC #13 lore raw

Re: [PATCH v2] New testcase to check if fallocate respects RLIMIT_FSIZE or not

Quoted 5 lines · Amit K. Arora

Quoted 3 lines · Eric Sandeen
Quoted 1 line · Amit K. Arora
Here is the new testcase.
Thanks! A few comments...
Thanks for the review!

Sure thing - looks good, I'll merge it after a retest if it all goes well. :)

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

[email protected]

maintainer

2010-05-21 01:11 UTC #20 lore raw

Re: [PATCH] ext4: Prevent creation of files larger than RLIMIT_FSIZE using fallocate

In reply to Amit K. Arora

Quoted 4 lines · Amit K. Arora

Quoted 2 lines · Nikanth Karthikesan
I assumed that Amit would send a patch with s-o-b, if not, please take this patch.
Have added my sign-off below. Thanks!

Added to the ext4 patch queue.

- Ted