do_sendfile ppos check ...

do_sendfile ppos check ...

[Herbert Poetzl]

Hi Andrew!

friend of mine stumbled over the following issue:

do_sendfile() does an overflow check near the end, like this:

        if (*ppos > max)
                retval = -EOVERFLOW;

now both sys_sendfile and sys_sendfile64 do call do_sendfile()
similar to this:

        if (offset) {
		...
                ret = do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
                return ret;
        }
	return do_sendfile(out_fd, in_fd, NULL, count, 0);

which passes ppos as NULL, which in turn leads to an oops ...

here is a patch (suggestion) to handle this properly, which
also adjusts the max for sys_sendfile()
(let me know what you think!)


--- linux-2.6.14/fs/read_write.c	2005-10-28 20:49:45 +0200
+++ linux-2.6.14-sendfile/fs/read_write.c	2005-11-03 18:48:37 +0100
@@ -731,7 +731,8 @@ asmlinkage ssize_t sys_sendfile(int out_
		return ret;
	}
 
-	return do_sendfile(out_fd, in_fd, NULL, count, 0);
+	pos = 0;
+	return do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
 }
 
 asmlinkage ssize_t sys_sendfile64(int out_fd, int in_fd, loff_t __user *offset, size_t count)
@@ -748,5 +749,6 @@ asmlinkage ssize_t sys_sendfile64(int ou
		return ret;
	}
 
-	return do_sendfile(out_fd, in_fd, NULL, count, 0);
+	pos = 0;
+	return do_sendfile(out_fd, in_fd, &pos, count, 0);
 }

Re: do_sendfile ppos check ...

[Herbert Xu]

Herbert Poetzl <herbert@13thfloor.at> wrote:
> 
> which passes ppos as NULL, which in turn leads to an oops ...

But do_sendfile will set ppos to &in_file->f_pos if it's NULL.
Why isn't that working?

> @@ -731,7 +731,8 @@ asmlinkage ssize_t sys_sendfile(int out_
>                return ret;
>        }
> 
> -       return do_sendfile(out_fd, in_fd, NULL, count, 0);
> +       pos = 0;
> +       return do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
> }

The last argument is meant to be zero if you check the history.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: do_sendfile ppos check ...

[Herbert Poetzl]

On Fri, Nov 04, 2005 at 01:36:36PM +1100, Herbert Xu wrote:
> Herbert Poetzl <herbert@13thfloor.at> wrote:
> > 
> > which passes ppos as NULL, which in turn leads to an oops ...
> 
> But do_sendfile will set ppos to &in_file->f_pos if it's NULL.
> Why isn't that working?
> 
> > @@ -731,7 +731,8 @@ asmlinkage ssize_t sys_sendfile(int out_
> >                return ret;
> >        }
> > 
> > -       return do_sendfile(out_fd, in_fd, NULL, count, 0);
> > +       pos = 0;
> > +       return do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
> > }
> 
> The last argument is meant to be zero if you check the history.

hmm, why a different max than with offset?

currently investigating ... probably a removal of
the 'unnecessary' check (*ppos) would be a better
approach, something like:

--- linux-2.6/fs/read_write.c   2005-10-28 23:59:02.000000000 +0200
+++ linux-2.6/fs/read_write.c   2005-11-03 17:28:50.000000000 +0100
@@ -719,9 +719,6 @@
       	current->syscr++;
       	current->syscw++;

-       if (*ppos > max)
-               retval = -EOVERFLOW;
-
 fput_out:
         	ds,   fput_light(out_file, fput_needed_out);
 fput_in:

thanks for the input,
Herbert

> 
> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: do_sendfile ppos check ...

[Herbert Xu]

On Fri, Nov 04, 2005 at 04:10:13AM +0100, Herbert Poetzl wrote:
> 
> currently investigating ... probably a removal of
> the 'unnecessary' check (*ppos) would be a better
> approach, something like:
> 
> --- linux-2.6/fs/read_write.c   2005-10-28 23:59:02.000000000 +0200
> +++ linux-2.6/fs/read_write.c   2005-11-03 17:28:50.000000000 +0100
> @@ -719,9 +719,6 @@
>        	current->syscr++;
>        	current->syscw++;
> 
> -       if (*ppos > max)
> -               retval = -EOVERFLOW;
> -

This still doesn't make sense.  If ppos came in as NULL, it should
have become non-NULL long before it reaches this part of the function.

Look at the top of the do_sendfile function, it sets ppos if it is NULL.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt