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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 01 Feb 2016 11:28:51 -0800
From:	Nikhilesh Reddy <reddyn@...eaurora.org>
To:	Jann Horn <jann@...jh.net>
CC:	torvalds@...ux-foundation.org, Miklos Szeredi <miklos@...redi.hu>,
	fuse-devel <fuse-devel@...ts.sourceforge.net>,
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	gregkh@...uxfoundation.org, linux-fsdevel@...r.kernel.org,
	viro@...iv.linux.org.uk, Richard Weinberger <richard@....at>,
	Theodore Ts'o <tytso@....edu>, jack@...e.cz,
	Antonio SJ Musumeci <trapexit@...wn.link>, sven.utcke@....de,
	Nikolaus Rath <nikolaus@...h.org>,
	Jann Horn <jannhorn@...glemail.com>,
	Mike Shal <marfey@...il.com>
Subject: Re: [PATCH v5] fuse: Add support for passthrough read/write

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.