From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikhilesh Reddy Subject: Re: [PATCH v5] fuse: Add support for passthrough read/write Date: Mon, 01 Feb 2016 11:28:51 -0800 Message-ID: <56AFB1F3.8090902@codeaurora.org> References: <56AFAA5B.3000006@codeaurora.org> <20160201191555.GA3524@pc.thejh.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160201191555.GA3524-J1fxOzX/cBvk1uMJSBkQmQ@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jann Horn Cc: torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org, Miklos Szeredi , fuse-devel , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org, Richard Weinberger , Theodore Ts'o , jack-AlSwsSmVLrQ@public.gmane.org, Antonio SJ Musumeci , sven.utcke-Mmb7MZpHnFY@public.gmane.org, Nikolaus Rath , Jann Horn , Mike Shal List-Id: linux-api@vger.kernel.org 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.