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

[Andrew Morton <akpm@linux-foundation.org>]

(Amit Arora <aarora@in.ibm.com> wrote fallocate.  cc added)

On Thu, 29 Apr 2010 10:14:06 +0530
Nikanth Karthikesan <knikanth@suse.de> 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.

> 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 <knikanth@suse.de>
> Reported-by: Eelis - <opensuse.org@contacts.eelis.net>
> 
> ---
> 
> diff --git a/fs/open.c b/fs/open.c
> index 74e5cd9..4ca57c9 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -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
> -	 * 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.

> +	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)) {
> +		/*
> +		 * 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.