[PATCH 0/3] vfs: change sb->s_maxbytes to loff_t (try #2)

[PATCH 0/3] vfs: change sb->s_maxbytes to loff_t (try #2)

[Jeff Layton]

This is the second attempt at a set to convert s_maxbytes to loff_t. The
first set had a patch for fiemap that doesn't seem to actually be needed
and removed a check from do_sendfile that does seem to be needed. This
set should fix those problems and also cleans up the warning in
vfs_kern_mount a bit.

Recently, I spent a day or so tracking down a long-standing problem with
CIFS and sendfile(). The problem turned out to be that CIFS was setting
s_maxbytes to a value that, when cast to a signed value, became
negative. This broke the offset checks in do_sendfile().

While I fixed this in CIFS, it turns out that this problem is actually a
little more widespread. Since setting s_maxbytes to a value larger than
MAX_LFS_FILESIZE breaks things, I believe it makes sense to turn
s_maxbytes into an loff_t. Most of the places that compare values to
s_maxbytes are comparing it against signed loff_t values, so this change
should help reduce the amount of implicit conversion that needs to be
done.

I've looked at the places that use s_maxbytes and didn't see any that
would immediately break with this change. That said, some of the
calculations that filesystems use to set s_maxbytes are convoluted, so
it's possible that I missed some. A critical eye toward that when
reviewing would be good.

This set is only lightly tested, but it's fairly straightforward. Many
thanks to those that have reviewed this so far.

Jeff Layton (3):
  vfs: make get_sb_pseudo set s_maxbytes to value that can be cast to
    signed
  vfs: remove redundant position check in do_sendfile
  vfs: change sb->s_maxbytes to a loff_t

 fs/libfs.c         |    2 +-
 fs/read_write.c    |    3 ---
 fs/super.c         |   10 ++++++++++
 include/linux/fs.h |    2 +-
 4 files changed, 12 insertions(+), 5 deletions(-)

[PATCH 1/3] vfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[Jeff Layton]

get_sb_pseudo sets s_maxbytes to ~0ULL which becomes negative when cast
to a signed value. Fix it to use MAX_LFS_FILESIZE which casts properly
to a positive signed value.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Steve French <smfrench@gmail.com>
---
 fs/libfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index ddfa899..dcec3d3 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -217,7 +217,7 @@ int get_sb_pseudo(struct file_system_type *fs_type, char *name,
 		return PTR_ERR(s);
 
 	s->s_flags = MS_NOUSER;
-	s->s_maxbytes = ~0ULL;
+	s->s_maxbytes = MAX_LFS_FILESIZE;
 	s->s_blocksize = PAGE_SIZE;
 	s->s_blocksize_bits = PAGE_SHIFT;
 	s->s_magic = magic;
-- 
1.6.0.6

Re: [PATCH 1/3] vfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[Christoph Hellwig]

On Mon, Aug 10, 2009 at 08:17:41AM -0400, Jeff Layton wrote:
> get_sb_pseudo sets s_maxbytes to ~0ULL which becomes negative when cast
> to a signed value. Fix it to use MAX_LFS_FILESIZE which casts properly
> to a positive signed value.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Steve French <smfrench@gmail.com>

Looks good, and I think it would be very good to still gets this into
2.6.31.

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

Re: [PATCH 1/3] vfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[Jeff Layton]

On Mon, 10 Aug 2009 09:49:50 -0400
Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Aug 10, 2009 at 08:17:41AM -0400, Jeff Layton wrote:
> > get_sb_pseudo sets s_maxbytes to ~0ULL which becomes negative when cast
> > to a signed value. Fix it to use MAX_LFS_FILESIZE which casts properly
> > to a positive signed value.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>
> > Acked-by: Steve French <smfrench@gmail.com>
> 
> Looks good, and I think it would be very good to still gets this into
> 2.6.31.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Agreed. The other two should probably go in early 2.6.32 (assuming
they're ok).

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/3] vfs: make get_sb_pseudo set s_maxbytes to value that can be cast to signed

[Christoph Hellwig]

On Mon, Aug 10, 2009 at 10:08:18AM -0400, Jeff Layton wrote:
> Agreed. The other two should probably go in early 2.6.32 (assuming
> they're ok).

Exactly.

[PATCH 2/3] vfs: remove redundant position check in do_sendfile

[Jeff Layton]

As Johannes Weiner pointed out, one of the range checks in do_sendfile
is redundant and is already checked in rw_verify_area.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/read_write.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 6c8c55d..3ac2898 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -839,9 +839,6 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
 
 	pos = *ppos;
-	retval = -EINVAL;
-	if (unlikely(pos < 0))
-		goto fput_out;
 	if (unlikely(pos + count > max)) {
 		retval = -EOVERFLOW;
 		if (pos >= max)
-- 
1.6.0.6

Re: [PATCH 2/3] vfs: remove redundant position check in do_sendfile

[Johannes Weiner]

On Mon, Aug 10, 2009 at 08:17:42AM -0400, Jeff Layton wrote:
> As Johannes Weiner pointed out, one of the range checks in do_sendfile
> is redundant and is already checked in rw_verify_area.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks!

> ---
>  fs/read_write.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 6c8c55d..3ac2898 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -839,9 +839,6 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
>  
>  	pos = *ppos;
> -	retval = -EINVAL;
> -	if (unlikely(pos < 0))
> -		goto fput_out;
>  	if (unlikely(pos + count > max)) {
>  		retval = -EOVERFLOW;
>  		if (pos >= max)
> -- 
> 1.6.0.6

[PATCH 3/3] vfs: change sb->s_maxbytes to a loff_t

[Jeff Layton]

sb->s_maxbytes is supposed to indicate the maximum size of a file that
can exist on the filesystem. It's declared as an unsigned long long.

Even if a filesystem has no inherent limit that prevents it from using
every bit in that unsigned long long, it's still problematic to set it
to anything larger than MAX_LFS_FILESIZE. There are places in the kernel
that cast s_maxbytes to a signed value. If it's set too large then this
cast makes it a negative number and generally breaks the comparison.

Change s_maxbytes to be loff_t instead. That should help eliminate the
temptation to set it too large by making it a signed value.

Also, add a warning for couple of releases to help catch filesystems
that set s_maxbytes too large. Eventually we can either convert this to
a BUG() or just remove it and in the hope that no one will get it wrong
now that it's a signed value.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/super.c         |   10 ++++++++++
 include/linux/fs.h |    2 +-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 2761d3e..660d437 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -889,6 +889,16 @@ vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void
  	if (error)
  		goto out_sb;
 
+	/*
+	 * filesystems should never set s_maxbytes larger than MAX_LFS_FILESIZE
+	 * but s_maxbytes was an unsigned long long for many releases. Throw
+	 * this warning for a little while to try and catch filesystems that
+	 * violate this rule. This warning should be either removed or
+	 * converted to a BUG() in 2.6.34. 
+	 */
+	WARN((mnt->mnt_sb->s_maxbytes < 0), "WARNING: %s set sb->s_maxbytes to "
+		"negative value (%lld)\n", type->name, mnt->mnt_sb->s_maxbytes);
+
 	mnt->mnt_mountpoint = mnt->mnt_root;
 	mnt->mnt_parent = mnt;
 	up_write(&mnt->mnt_sb->s_umount);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 67888a9..bfe3c08 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1316,7 +1316,7 @@ struct super_block {
 	unsigned long		s_blocksize;
 	unsigned char		s_blocksize_bits;
 	unsigned char		s_dirt;
-	unsigned long long	s_maxbytes;	/* Max file size */
+	loff_t			s_maxbytes;	/* Max file size */
 	struct file_system_type	*s_type;
 	const struct super_operations	*s_op;
 	struct dquot_operations	*dq_op;
-- 
1.6.0.6

Re: [PATCH 3/3] vfs: change sb->s_maxbytes to a loff_t

[Mandeep Singh Baines]

Jeff Layton (jlayton@redhat.com) wrote:
> sb->s_maxbytes is supposed to indicate the maximum size of a file that
> can exist on the filesystem. It's declared as an unsigned long long.
> 
> Even if a filesystem has no inherent limit that prevents it from using
> every bit in that unsigned long long, it's still problematic to set it
> to anything larger than MAX_LFS_FILESIZE. There are places in the kernel
> that cast s_maxbytes to a signed value. If it's set too large then this
> cast makes it a negative number and generally breaks the comparison.
> 
> Change s_maxbytes to be loff_t instead. That should help eliminate the
> temptation to set it too large by making it a signed value.
> 
> Also, add a warning for couple of releases to help catch filesystems
> that set s_maxbytes too large. Eventually we can either convert this to
> a BUG() or just remove it and in the hope that no one will get it wrong
> now that it's a signed value.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/super.c         |   10 ++++++++++
>  include/linux/fs.h |    2 +-
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 2761d3e..660d437 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -889,6 +889,16 @@ vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void
>   	if (error)
>   		goto out_sb;
>  
> +	/*
> +	 * filesystems should never set s_maxbytes larger than MAX_LFS_FILESIZE
> +	 * but s_maxbytes was an unsigned long long for many releases. Throw
> +	 * this warning for a little while to try and catch filesystems that
> +	 * violate this rule. This warning should be either removed or
> +	 * converted to a BUG() in 2.6.34. 
> +	 */
> +	WARN((mnt->mnt_sb->s_maxbytes < 0), "WARNING: %s set sb->s_maxbytes to "
> +		"negative value (%lld)\n", type->name, mnt->mnt_sb->s_maxbytes);
> +

Minor nit. "WARNING:" is redundant. I believe WARN() will already print
"WARNING: at <file>:<line>".

>  	mnt->mnt_mountpoint = mnt->mnt_root;
>  	mnt->mnt_parent = mnt;
>  	up_write(&mnt->mnt_sb->s_umount);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 67888a9..bfe3c08 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1316,7 +1316,7 @@ struct super_block {
>  	unsigned long		s_blocksize;
>  	unsigned char		s_blocksize_bits;
>  	unsigned char		s_dirt;
> -	unsigned long long	s_maxbytes;	/* Max file size */
> +	loff_t			s_maxbytes;	/* Max file size */

Looks good to me. Thanks!

>  	struct file_system_type	*s_type;
>  	const struct super_operations	*s_op;
>  	struct dquot_operations	*dq_op;
> -- 
> 1.6.0.6
>