Re: [PATCH 07/23] vfs: Teach sendfile,splice,tee,and vmsplice to use file_hotplug_lock

Eric W. Biederman
2009-06-05T19:37:30Z

Badari Pulavarty <pbadari@gmail.com> writes:

> On Mon, 2009-06-01 at 14:50 -0700, Eric W. Biederman wrote:
>> From: Eric W. Biederman <ebiederm@maxwell.arastra.com>
>>
>> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>> ---
>> fs/read_write.c | 28 +++++++++----
>> fs/splice.c | 111 +++++++++++++++++++++++++++++++++++++-----------------
>> 2 files changed, 95 insertions(+), 44 deletions(-)
>>
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 718baea..c473d74 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -861,21 +861,24 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>> goto out;
>> if (!(in_file->f_mode & FMODE_READ))
>> goto fput_in;
>> + retval = -EIO;
>> + if (!file_hotplug_read_trylock(in_file))
>> + goto fput_in;
>> retval = -EINVAL;
>> in_inode = in_file->f_path.dentry->d_inode;
>> if (!in_inode)
>> - goto fput_in;
>> + goto unlock_in;
>> if (!in_file->f_op || !in_file->f_op->splice_read)
>> - goto fput_in;
>> + goto unlock_in;
>> retval = -ESPIPE;
>> if (!ppos)
>> ppos = &in_file->f_pos;
>> else
>> if (!(in_file->f_mode & FMODE_PREAD))
>> - goto fput_in;
>> + goto unlock_in;
>> retval = rw_verify_area(READ, in_file, ppos, count);
>> if (retval < 0)
>> - goto fput_in;
>> + goto unlock_in;
>> count = retval;
>>
>> /*
>> @@ -884,16 +887,19 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>> retval = -EBADF;
>> out_file = fget_light(out_fd, &fput_needed_out);
>> if (!out_file)
>> - goto fput_in;
>> + goto unlock_in;
>> if (!(out_file->f_mode & FMODE_WRITE))
>> goto fput_out;
>> + retval = -EIO;
>> + if (!file_hotplug_read_trylock(out_file))
>> + goto fput_out;
>> retval = -EINVAL;
>> if (!out_file->f_op || !out_file->f_op->sendpage)
>> - goto fput_out;
>> + goto unlock_out;
>> out_inode = out_file->f_path.dentry->d_inode;
>> retval = rw_verify_area(WRITE, out_file, &out_file->f_pos, count);
>> if (retval < 0)
>> - goto fput_out;
>> + goto unlock_out;
>> count = retval;
>>
>> if (!max)
>> @@ -902,11 +908,11 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>> pos = *ppos;
>> retval = -EINVAL;
>> if (unlikely(pos < 0))
>> - goto fput_out;
>> + goto unlock_out;
>> if (unlikely(pos + count > max)) {
>> retval = -EOVERFLOW;
>> if (pos >= max)
>> - goto fput_out;
>> + goto unlock_out;
>> count = max - pos;
>> }
>>
>> @@ -933,8 +939,12 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>> if (*ppos > max)
>> retval = -EOVERFLOW;
>>
>> +unlock_out:
>> + file_hotplug_read_unlock(out_file);
>> fput_out:
>> fput_light(out_file, fput_needed_out);
>> +unlock_in:
>> + file_hotplug_read_unlock(in_file);
>> fput_in:
>> fput_light(in_file, fput_needed_in);
>> out:
>> diff --git a/fs/splice.c b/fs/splice.c
>> index 666953d..fc6b3a5 100644
>> --- a/fs/splice.c
>> +++ b/fs/splice.c
>> @@ -1464,15 +1464,21 @@ SYSCALL_DEFINE4(vmsplice, int, fd, const struct iovec __user *, iov,
>>
>> error = -EBADF;
>> file = fget_light(fd, &fput);
>> - if (file) {
>> - if (file->f_mode & FMODE_WRITE)
>> - error = vmsplice_to_pipe(file, iov, nr_segs, flags);
>> - else if (file->f_mode & FMODE_READ)
>> - error = vmsplice_to_user(file, iov, nr_segs, flags);
>> + if (!file)
>> + goto out;
>>
>> - fput_light(file, fput);
>> - }
>> + if (!file_hotplug_read_trylock(file))
>> + goto fput_file;
>>
>> + if (file->f_mode & FMODE_WRITE)
>> + error = vmsplice_to_pipe(file, iov, nr_segs, flags);
>> + else if (file->f_mode & FMODE_READ)
>> + error = vmsplice_to_user(file, iov, nr_segs, flags);
>> +
>> + file_hotplug_read_unlock(file);
>> +fput_file:
>> + fput_light(file, fput);
>> +out:
>> return error;
>> }
>>
>> @@ -1489,21 +1495,39 @@ SYSCALL_DEFINE6(splice, int, fd_in, loff_t __user *, off_in,
>>
>> error = -EBADF;
>> in = fget_light(fd_in, &fput_in);
>> - if (in) {
>> - if (in->f_mode & FMODE_READ) {
>> - out = fget_light(fd_out, &fput_out);
>> - if (out) {
>> - if (out->f_mode & FMODE_WRITE)
>> - error = do_splice(in, off_in,
>> - out, off_out,
>> - len, flags);
>> - fput_light(out, fput_out);
>> - }
>> - }
>> + if (!in)
>> + goto out;
>>
>> - fput_light(in, fput_in);
>> - }
>> + if (!(in->f_mode & FMODE_READ))
>> + goto fput_in;
>> +
>> + error = -EIO;
>> + if (!file_hotplug_read_trylock(in))
>> + goto fput_in;
>> +
>> + error = -EBADF;
>> + out = fget_light(fd_out, &fput_out);
>> + if (!out)
>> + goto unlock_in;
>> +
>> + if (!(out->f_mode & FMODE_WRITE))
>> + goto fput_out;
>> +
>> + error = -EIO;
>> + if (!file_hotplug_read_trylock(out))
>> + goto fput_out;
>> +
>> + error = do_splice(in, off_in, out, off_out, len, flags);
>>
>> + file_hotplug_read_unlock(out);
>> +fput_out:
>> + fput_light(out, fput_out);
>> +unlock_in:
>> + file_hotplug_read_unlock(in);
>> +fput_in:
>> + fput_light(in, fput_in);
>> +
>> +out:
>> return error;
>> }
>>
>> @@ -1703,27 +1727,44 @@ static long do_tee(struct file *in, struct file *out, size_t len,
>>
>> SYSCALL_DEFINE4(tee, int, fdin, int, fdout, size_t, len, unsigned int, flags)
>> {
>> - struct file *in;
>> - int error, fput_in;
>> + struct file *in, *out;
>> + int error, fput_in, fput_out;
>>
>> if (unlikely(!len))
>> return 0;
>>
>> error = -EBADF;
>> in = fget_light(fdin, &fput_in);
>> - if (in) {
>> - if (in->f_mode & FMODE_READ) {
>> - int fput_out;
>> - struct file *out = fget_light(fdout, &fput_out);
>> -
>> - if (out) {
>> - if (out->f_mode & FMODE_WRITE)
>> - error = do_tee(in, out, len, flags);
>> - fput_light(out, fput_out);
>> - }
>> - }
>> - fput_light(in, fput_in);
>> - }
>> + if (!in)
>> + goto out;
>> +
>> + if (!(in->f_mode & FMODE_READ))
>> + goto unlock_in; <<<<<<<
>
> Shouldn't this be
> goto fput_in;

Good point. That is a bug.

> ? btw, its confusing to have labels and variables with same name:
> fput_in and fput_out. You may want to rename labels ?

Mayhap. I didn't start that one, although I am clearly spreading
it around here. Do you have a better naming suggestion?

>> + error = -EIO;
>> + if (!file_hotplug_read_trylock(in))
>> + goto fput_in;
>> +
>> + error = -EBADF;
>> + out = fget_light(fdout, &fput_out);
>> + if (!out)
>> + goto unlock_in;
>> +
>> + if (!(out->f_mode & FMODE_WRITE))
>> + goto fput_out;
>> +
>> + if (!file_hotplug_read_trylock(out))
>> + goto fput_out;
>> +
>> + error = do_tee(in, out, len, flags);
>> +
>> + file_hotplug_read_unlock(out);
>> +fput_out:
>> + fput_light(out, fput_out);
>> +unlock_in:
>> + file_hotplug_read_unlock(in);
>> +fput_in:
>> + fput_light(in, fput_in);
>> +out:
>> return error;
>> }

Eric