On Thu, Nov 03, 2005 at 09:01:14PM +0100, Grzegorz Nosek wrote:
> 2005/11/3, Herbert Poetzl <[EMAIL PROTECTED]>:
> > On Thu, Nov 03, 2005 at 05:38:43PM +0100, Grzegorz Nosek wrote:
> > > Hello all
> > >
> > > I needed to apply the patch below in order to keep the kernel from
> > > oopsing (in some older revisions) or freezing solid (in the newest,
> > > listed in the subject.
> >
> > as follow up, please try the following patch instead:
> >
> > http://lkml.org/lkml/diff/2005/11/3/161/1
> >
> > rationale: http://lkml.org/lkml/2005/11/3/161
> >
> > HTH,
> > Herbert
> >
> >
>
> Hello,
>
> I think I disagree that my proposed patch is hiding the real bug
> because the ppos and max variables aren't ever used in do_sendfile.
> They're blindly passed to vfs_sendfile, which does the checks and
> returns -EOVERFLOW if needed.
hmm, good that you disagree ...
> IMHO my patch is more of The Right Way (tm) because it doesn't involve
> modifications in another function (only do_sendfile is affected, which
> is modified heavily anyway).
hmm, well, maybe a mix of both approaches is what we
really want because the following looks 'suspicious'
to me:
asmlinkage ssize_t sys_sendfile(int out_fd, int in_fd,
off_t __user *offset, size_t count)
{
...
if (offset) {
...
pos = off;
ret = do_sendfile(out_fd, in_fd, &pos, count, MAX_NON_LFS);
...
return ret;
}
return do_sendfile(out_fd, in_fd, NULL, count, 0);
}
because in the case we _have_ an offset specified, we
pass a pos and limit (MAX_NON_LFS) and if not, we just
set it to zero?
> Of course, you're free to either include it or ignore it.
>
> (my definitions of vfs_sendfile and do_sendfile with some random
> annotations below)
well, I agree with you that the check in do_sendfile()
is superfluous ... and that the checks in vfs_sendfile()
should be sufficient ... haven't checked all cases here
best,
Herbert
PS: please follow up on lkml, if not done so already ...
> ssize_t vfs_sendfile(struct file *out_file, struct file *in_file, loff_t
> *ppos,
> size_t count, loff_t max)
> {
> struct inode * in_inode, * out_inode;
> loff_t pos;
> ssize_t ret;
>
> /* verify in_file */
> in_inode = in_file->f_dentry->d_inode;
> if (!in_inode)
> return -EINVAL;
> if (!in_file->f_op || !in_file->f_op->sendfile)
> return -EINVAL;
>
> // !!! ppos is validated here
> if (!ppos)
> ppos = &in_file->f_pos;
> else
> if (!(in_file->f_mode & FMODE_PREAD))
> return -ESPIPE;
>
> ret = rw_verify_area(FLOCK_VERIFY_READ, in_file, ppos, count);
> if (ret)
> return ret;
>
> /* verify out_file */
> out_inode = out_file->f_dentry->d_inode;
> if (!out_inode)
> return -EINVAL;
> if (!out_file->f_op || !out_file->f_op->sendpage)
> return -EINVAL;
>
> ret = rw_verify_area(FLOCK_VERIFY_WRITE, out_file,
> &out_file->f_pos, count);
> if (ret)
> return ret;
>
> ret = security_file_permission (out_file, MAY_WRITE);
> if (ret)
> return ret;
>
> // !!! max is validated here
> if (!max)
> max = min(in_inode->i_sb->s_maxbytes,
> out_inode->i_sb->s_maxbytes);
>
> pos = *ppos;
> if (unlikely(pos < 0))
> return -EINVAL;
> if (unlikely(pos + count > max)) {
> if (pos >= max)
> return -EOVERFLOW;
> count = max - pos;
> }
>
> ret = in_file->f_op->sendfile(in_file, ppos, count,
> file_send_actor, out_file);
>
> if (*ppos > max)
> return -EOVERFLOW;
> return ret;
> }
>
> EXPORT_SYMBOL(vfs_sendfile);
>
> static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
> size_t count, loff_t max)
> {
> struct file * in_file, * out_file;
> ssize_t retval;
> int fput_needed_in, fput_needed_out;
>
> /*
> * Get input file, and verify that it is ok..
> */
> retval = -EBADF;
> in_file = fget_light(in_fd, &fput_needed_in);
> if (!in_file)
> goto out;
> if (!(in_file->f_mode & FMODE_READ))
> goto fput_in;
>
> retval = security_file_permission (in_file, MAY_READ);
> if (retval)
> goto fput_in;
>
> /*
> * Get output file, and verify that it is ok..
> */
> retval = -EBADF;
> out_file = fget_light(out_fd, &fput_needed_out);
> if (!out_file)
> goto fput_in;
> if (!(out_file->f_mode & FMODE_WRITE))
> goto fput_out;
>
> retval = vfs_sendfile(out_file, in_file, ppos, count, max);
>
> // !!! if vfs_sendfile returned -EOVERFLOW, it propagates out of
> do_sendfile too (and doesn't skip fput_XXX)
>
> if (retval > 0) {
> current->rchar += retval;
> current->wchar += retval;
> }
> current->syscr++;
> current->syscw++;
>
> fput_out:
> fput_light(out_file, fput_needed_out);
> fput_in:
> fput_light(in_file, fput_needed_in);
> out:
> return retval;
> }
> _______________________________________________
> Vserver mailing list
> [email protected]
> http://list.linux-vserver.org/mailman/listinfo/vserver
_______________________________________________
Vserver mailing list
[email protected]
http://list.linux-vserver.org/mailman/listinfo/vserver
