sendfile bugs(?)

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.

@@ -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);

@@ -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:

@@ -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)

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.

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