[patch 09/12] sendfile: several fixes

[patch 09/12] sendfile: several fixes

[akpm]

From: Mandeep Singh Baines <msb@google.com>

Three fixes for sendfile, mostly related to sending large files from pseudo
filesystems:

- Fix sendfile for offsets > 300G. This can happen with pseudo filesystems.
  This happens because the overflow check is using inode->i_sb->s_maxbytes
  and not the superblock of the backing device's s_maxbytes. For a regular file
  these are interchangible but for a special file these are different and you
  want the latter.

- Don't compare against the max of the out_inode's superblock. Doesn't make
  sense.

- For pseudo and other filesystems with s_maxbytes set to ~0ULL, max ends up
  holding a negative number as it is signed. Check for and correct that.

Signed-off-by: Robert Love <rlove@google.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/read_write.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff -puN fs/read_write.c~sendfile-several-fixes fs/read_write.c
--- a/fs/read_write.c~sendfile-several-fixes
+++ a/fs/read_write.c
@@ -835,8 +835,15 @@ static ssize_t do_sendfile(int out_fd, i
 		goto fput_out;
 	count = retval;
 
-	if (!max)
-		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
+	if (!max) {
+		max = in_inode->i_mapping->host->i_sb->s_maxbytes;
+		/*
+		 * For psuedo filesystems, s_maxbytes is ~0ULL. When converted
+		 * to loff_t, it can go negative. So we check for and fix that.
+		 */
+		if (max < 0)
+			max = LLONG_MAX;
+	}
 
 	pos = *ppos;
 	retval = -EINVAL;
_

Re: [patch 09/12] sendfile: several fixes

[Christoph Hellwig]

On Thu, Aug 06, 2009 at 04:10:15PM -0700, akpm@linux-foundation.org wrote:
> From: Mandeep Singh Baines <msb@google.com>
> 
> Three fixes for sendfile, mostly related to sending large files from pseudo
> filesystems:
> 
> - Fix sendfile for offsets > 300G. This can happen with pseudo filesystems.
>   This happens because the overflow check is using inode->i_sb->s_maxbytes
>   and not the superblock of the backing device's s_maxbytes. For a regular file
>   these are interchangible but for a special file these are different and you
>   want the latter.
> 
> - Don't compare against the max of the out_inode's superblock. Doesn't make
>   sense.
> 
> - For pseudo and other filesystems with s_maxbytes set to ~0ULL, max ends up
>   holding a negative number as it is signed. Check for and correct that.

The right fix is to not set s_maxbytes to ~0ULL.  Jeff Layton has sent
some patches for that recently.

Re: [patch 09/12] sendfile: several fixes

[Robert Love]

On Thu, Aug 6, 2009 at 7:24 PM, Christoph Hellwig<hch@infradead.org> wrote:

> The right fix is to not set s_maxbytes to ~0ULL.  Jeff Layton has sent
> some patches for that recently.

I agree.

I still think this check is worthwhile, given the signed/unsigned mismatch.

       Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 09/12] sendfile: several fixes

[Christoph Hellwig]

On Thu, Aug 06, 2009 at 07:25:56PM -0400, Robert Love wrote:
> On Thu, Aug 6, 2009 at 7:24 PM, Christoph Hellwig<hch@infradead.org> wrote:
> 
> > The right fix is to not set s_maxbytes to ~0ULL. ?Jeff Layton has sent
> > some patches for that recently.
> 
> I agree.
> 
> I still think this check is worthwhile, given the signed/unsigned mismatch.

I think the plan is to change s_maxbytes to be an loff_t which would
eliminate this.  I'm not sure if Jeff has sent that patch already.

Re: [patch 09/12] sendfile: several fixes

[Mandeep Singh Baines]

Christoph Hellwig (hch@infradead.org) wrote:
> On Thu, Aug 06, 2009 at 07:25:56PM -0400, Robert Love wrote:
> > On Thu, Aug 6, 2009 at 7:24 PM, Christoph Hellwig<hch@infradead.org> wrote:
> > 
> > > The right fix is to not set s_maxbytes to ~0ULL. ?Jeff Layton has sent
> > > some patches for that recently.
> > 

Yeah, found it here:

http://lkml.org/lkml/2009/7/22/178

> > I agree.
> > 
> > I still think this check is worthwhile, given the signed/unsigned mismatch.
> 
> I think the plan is to change s_maxbytes to be an loff_t which would
> eliminate this.  I'm not sure if Jeff has sent that patch already.
> 

Yeah, found the discussion here:

http://lkml.org/lkml/2009/7/22/241

Cool. Didn't really like this check. It would be nice to remove the check
and comment from the next version of this patch.

Re: [patch 09/12] sendfile: several fixes

[Andrew Morton]

On Thu, 6 Aug 2009 23:12:17 -0700 Mandeep Singh Baines <msb@google.com> wrote:

> Christoph Hellwig (hch@infradead.org) wrote:
> > On Thu, Aug 06, 2009 at 07:25:56PM -0400, Robert Love wrote:
> > > On Thu, Aug 6, 2009 at 7:24 PM, Christoph Hellwig<hch@infradead.org> wrote:
> > > 
> > > > The right fix is to not set s_maxbytes to ~0ULL. ?Jeff Layton has sent
> > > > some patches for that recently.
> > > 
> 
> Yeah, found it here:
> 
> http://lkml.org/lkml/2009/7/22/178
> 
> > > I agree.
> > > 
> > > I still think this check is worthwhile, given the signed/unsigned mismatch.
> > 
> > I think the plan is to change s_maxbytes to be an loff_t which would
> > eliminate this.  I'm not sure if Jeff has sent that patch already.
> > 
> 
> Yeah, found the discussion here:
> 
> http://lkml.org/lkml/2009/7/22/241
> 
> Cool. Didn't really like this check. It would be nice to remove the check
> and comment from the next version of this patch.

Jeff, can you please redo/refresh/review/resend that stuff?

Thanks.

Re: [patch 09/12] sendfile: several fixes

[Jeff Layton]

On Thu, 6 Aug 2009 23:26:55 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 6 Aug 2009 23:12:17 -0700 Mandeep Singh Baines <msb@google.com> wrote:
> 
> > Christoph Hellwig (hch@infradead.org) wrote:
> > > On Thu, Aug 06, 2009 at 07:25:56PM -0400, Robert Love wrote:
> > > > On Thu, Aug 6, 2009 at 7:24 PM, Christoph Hellwig<hch@infradead.org> wrote:
> > > > 
> > > > > The right fix is to not set s_maxbytes to ~0ULL. ?Jeff Layton has sent
> > > > > some patches for that recently.
> > > > 
> > 
> > Yeah, found it here:
> > 
> > http://lkml.org/lkml/2009/7/22/178
> > 
> > > > I agree.
> > > > 
> > > > I still think this check is worthwhile, given the signed/unsigned mismatch.
> > > 
> > > I think the plan is to change s_maxbytes to be an loff_t which would
> > > eliminate this.  I'm not sure if Jeff has sent that patch already.
> > > 
> > 
> > Yeah, found the discussion here:
> > 
> > http://lkml.org/lkml/2009/7/22/241
> > 
> > Cool. Didn't really like this check. It would be nice to remove the check
> > and comment from the next version of this patch.
> 
> Jeff, can you please redo/refresh/review/resend that stuff?
> 

Yep. I'll try to have something next week. The tricky part here is
checking all of the existing filesystems and making sure that changing
s_maxbytes to loff_t won't break anything.

I also want to clean up the checks in do_sendfile like Johannes
suggested, though I still have some questions about that...

-- 
Jeff Layton <jlayton@redhat.com>