Re: [PATCH v5] fuse: Add support for passthrough read/write

[Nikhilesh Reddy]

On Mon 01 Feb 2016 11:15:56 AM PST, Jann Horn wrote:
> On Mon, Feb 01, 2016 at 10:56:27AM -0800, Nikhilesh Reddy wrote:
> > diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
> [...]
> > +static ssize_t fuse_passthrough_read_write_iter(struct kiocb *iocb,
> > +					    struct iov_iter *iter, int do_write)
> > +{
> > +	ssize_t ret_val;
> > +	struct fuse_file *ff;
> > +	struct file *fuse_file, *passthrough_filp;
> > +	struct inode *fuse_inode, *passthrough_inode;
> > +
> > +	ff = iocb->ki_filp->private_data;
> > +	fuse_file = iocb->ki_filp;
> > +	passthrough_filp = ff->passthrough_filp;
> > +
> > +	/* lock passthrough file to prevent it from being released */
> > +	get_file(passthrough_filp);
> > +	iocb->ki_filp = passthrough_filp;
> > +	fuse_inode = fuse_file->f_path.dentry->d_inode;
> > +	passthrough_inode = file_inode(passthrough_filp);
> > +
> > +	if (do_write) {
> > +		if (!passthrough_filp->f_op->write_iter)
> > +			return -EIO;
> > +		ret_val = passthrough_filp->f_op->write_iter(iocb, iter);
> > +
> > +		if (ret_val >= 0 || ret_val == -EIOCBQUEUED) {
> > +			fsstack_copy_inode_size(fuse_inode, passthrough_inode);
> > +			fsstack_copy_attr_times(fuse_inode, passthrough_inode);
> > +		}
> > +	} else {
> > +		if (!passthrough_filp->f_op->read_iter)
> > +			return -EIO;
> > +		ret_val = passthrough_filp->f_op->read_iter(iocb, iter);
> > +		if (ret_val >= 0 || ret_val == -EIOCBQUEUED)
> > +			fsstack_copy_attr_atime(fuse_inode, passthrough_inode);
> > +	}
> > +
> > +	iocb->ki_filp = fuse_file;
> > +
> > +	/* unlock passthrough file */
> > +	fput(passthrough_filp);
>
> Why the get_file() and fput() in this method? This doesn't look right. There
> is no lock you're releasing between get_file() and fput(). What are they
> intended for?

Hi

Thanks for reviewing the code.

The passthrough file could be released under our feet say  if the 
userspace fuse daemon crashed or was killed  ( while we are processing 
the read or the write) causing bad things to happen.
The calls here are to increase the count temporarily  and then decrease 
it so that we dont release in the middle of a write and everything is 
gracefully handled...

I have a comment right before the get_file call above saying the same 
thing.
Please let me know if you have any more questions.


--
Thanks
Nikhilesh Reddy

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project.