sendfile bugs(?)

sendfile bugs(?)

[Prasanna Meda]

There seems to be couple of issues in sendfile code.
   - read_write.c: sys_sendfile:do_sendfile checks for ppos
       to be NULL,  the intension is checking the contents for
       zero.
  - locks_verify_write() may checks at different offset for
      permission, and copy at different offset, since it is also
      using out_file->f_pos.  And also it will be cleaner to
      update out_file->f_pos atomically only when it succeeds.
      It also deletes the dependency on f_pos from filemap.c.

Thanks,
Prasanna.

--- fs/read_write.c.saved       Tue Aug 10 19:10:35 2004
+++ fs/read_write.c     Tue Aug 10 19:34:09 2004
@@ -546,6 +546,7 @@
        loff_t pos;
        ssize_t retval;
        int fput_needed_in, fput_needed_out;
+       struct sendfile_target target;

        /*
         * Get input file, and verify that it is ok..
@@ -562,7 +563,7 @@
                goto fput_in;
        if (!in_file->f_op || !in_file->f_op->sendfile)
                goto fput_in;
-       if (!ppos)
+       if (!*ppos)
                ppos = &in_file->f_pos;
        retval = locks_verify_area(FLOCK_VERIFY_READ, in_inode, in_file,
*ppos, count);
        if (retval)
@@ -585,7 +586,10 @@
        if (!out_file->f_op || !out_file->f_op->sendpage)
                goto fput_out;
        out_inode = out_file->f_dentry->d_inode;
-       retval = locks_verify_area(FLOCK_VERIFY_WRITE, out_inode,
out_file, out_file->f_pos, count);
+
+       target.file = out_file;
+       target.pos = out_file->f_pos;
+       retval = locks_verify_area(FLOCK_VERIFY_WRITE, out_inode,
out_file, target.pos, count);
        if (retval)
                goto fput_out;

@@ -607,10 +611,12 @@
                count = max - pos;
        }

-       retval = in_file->f_op->sendfile(in_file, ppos, count,
file_send_actor, out_file);
+       retval = in_file->f_op->sendfile(in_file, ppos, count,
file_send_actor, &target);

        if (*ppos > max)
                retval = -EOVERFLOW;
+       if (!retval)
+               out_file->f_pos = target.pos;

 fput_out:
        fput_light(out_file, fput_needed_out);
--- include/linux/fs.h.saved    Tue Aug 10 19:17:05 2004
+++ include/linux/fs.h  Tue Aug 10 19:24:53 2004
@@ -851,9 +851,15 @@
        size_t count;
        char __user * buf;
        int error;
+       loff_t *pos;
 } read_descriptor_t;

 typedef int (*read_actor_t)(read_descriptor_t *, struct page *,
unsigned long, unsigned long);
+
+struct sendfile_target {
+       struct file *file;
+       loff_t pos;
+};

 /*
  * NOTE:
--- mm/filemap.c.saved  Tue Aug 10 19:13:07 2004
+++ mm/filemap.c        Tue Aug 10 19:24:35 2004
@@ -941,13 +941,14 @@
 {
        ssize_t written;
        unsigned long count = desc->count;
-       struct file *file = (struct file *) desc->buf;
+       struct file *file = desc->buf;
+       loff_t *ppos = desc->pos;

        if (size > count)
                size = count;

        written = file->f_op->sendpage(file, page, offset,
-                                      size, &file->f_pos, size<count);
+                                      size, ppos, size<count);
        if (written < 0) {
                desc->error = written;
                written = 0;
@@ -958,17 +959,19 @@
 }

 ssize_t generic_file_sendfile(struct file *in_file, loff_t *ppos,
-                        size_t count, read_actor_t actor, void __user
*target)
+                        size_t count, read_actor_t actor, void __user
*arg)
 {
        read_descriptor_t desc;
+       struct sendfile_target *target = arg;

        if (!count)
                return 0;

        desc.written = 0;
        desc.count = count;
-       desc.buf = target;
+       desc.buf = target->file;
        desc.error = 0;
+       desc.pos = &target->pos;

        do_generic_file_read(in_file, ppos, &desc, actor);
        if (desc.written)

Re: sendfile bugs(?)

[Linus Torvalds]

On Tue, 10 Aug 2004, Prasanna Meda wrote:
> 
> There seems to be couple of issues in sendfile code.
>    - read_write.c: sys_sendfile:do_sendfile checks for ppos
>        to be NULL,  the intension is checking the contents for
>        zero.

No, it really checks the _pointer_ for NULL. 

A NULL ptr means that it's not a pread interface, but that we should use 
f_pos. It's for when the user doesn't pass in any ppos thing: see 
sys_sendfile() for the two cases.

>   - locks_verify_write() may checks at different offset for
>       permission, and copy at different offset, since it is also
>       using out_file->f_pos.  And also it will be cleaner to
>       update out_file->f_pos atomically only when it succeeds.
>       It also deletes the dependency on f_pos from filemap.c.

Yes, this part is probably worth doing. It's probably _also_ worth doing
something similar for the in-file f_pos thing (ie pass in a copy of f_pos
the same way read/write does these days).

		Linus