ecryptfs-devel team mailing list archive

[Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxx>]

On 09/18/2009 04:50 PM, Serge E. Hallyn wrote:
> Quoting Tyler Hicks (tyhicks@xxxxxxxxxxxxxxxxxx):
>> If the lower inode is read-only, don't attempt to open the lower file
>> read/write and don't hand off the open request to the privileged
>> eCryptfs kthread for opening it read/write.  Instead, only try an
>> unprivileged, read-only open of the file and give up if that fails.
>> This patch fixes an oops when eCryptfs is mounted on top of a read-only
>> mount.
>>
>> Cc: Serge Hallyn <serue@xxxxxxxxxx>
>> Cc: Eric Sandeen <esandeen@xxxxxxxxxx>
>> Cc: Dave Kleikamp <shaggy@xxxxxxxxxxxxxxxxxx>
>> Cc: ecryptfs-devel@xxxxxxxxxxxxxxxxxxx
>> Signed-off-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxx>
> 
> They all look ok to me, with about 20 mins of looking over.

Thanks for taking the time!

> 
> This one also seems to fix a problem at the caller which was
> taking PTR_ERR(lower_file) as its rc, even though the code
> you're removing here was setting lower_file to NULL on failure.
> 
> No, wait.  If the second step fails, then you still keep
> lower_file as an open file instead of closing it and
> setting it to ERR_PTR(rc)...

I'm not quite following this.  What is the second step that you're
referring to?  The privileged kthread open in ecryptfs_threadfn()?  If
that fails, then the file won't be open.

> It's possible I'm on an older
> tree than you are, so can you please re-confirm that
> ecryptfs_init_persistent_file()'s usage of rc and its
> 	rc = PTR_ERR(inode_info->lower_file;
> jives with what you do in ecryptfs_privileged_open()?

You're right, the handling of rc in the caller is pretty kludgy.  It can
just use the return code of ecryptfs_privileged_open() and not mess
inode_info->lower_file upon failure.

I'll respond with a patch.

> 
> Still,
> 
> Acked-by: Serge Hallyn <serue@xxxxxxxxxx>
> 
> for the set.
> 
> thanks,
> -serge
> 
>> ---
>>  fs/ecryptfs/kthread.c |   24 ++++++++----------------
>>  1 files changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/ecryptfs/kthread.c b/fs/ecryptfs/kthread.c
>> index c6d7a4d..e14cf7e 100644
>> --- a/fs/ecryptfs/kthread.c
>> +++ b/fs/ecryptfs/kthread.c
>> @@ -136,6 +136,7 @@ int ecryptfs_privileged_open(struct file **lower_file,
>>  			     const struct cred *cred)
>>  {
>>  	struct ecryptfs_open_req *req;
>> +	int flags = O_LARGEFILE;
>>  	int rc = 0;
>>
>>  	/* Corresponding dput() and mntput() are done when the
>> @@ -143,10 +144,14 @@ int ecryptfs_privileged_open(struct file **lower_file,
>>  	 * destroyed. */
>>  	dget(lower_dentry);
>>  	mntget(lower_mnt);
>> -	(*lower_file) = dentry_open(lower_dentry, lower_mnt,
>> -				    (O_RDWR | O_LARGEFILE), cred);
>> +	flags |= IS_RDONLY(lower_dentry->d_inode) ? O_RDONLY : O_RDWR;
>> +	(*lower_file) = dentry_open(lower_dentry, lower_mnt, flags, cred);
>>  	if (!IS_ERR(*lower_file))
>>  		goto out;
>> +	if (flags & O_RDONLY) {
>> +		rc = PTR_ERR((*lower_file));
>> +		goto out;
>> +	}
>>  	req = kmem_cache_alloc(ecryptfs_open_req_cache, GFP_KERNEL);
>>  	if (!req) {
>>  		rc = -ENOMEM;
>> @@ -180,21 +185,8 @@ int ecryptfs_privileged_open(struct file **lower_file,
>>  		       __func__);
>>  		goto out_unlock;
>>  	}
>> -	if (IS_ERR(*req->lower_file)) {
>> +	if (IS_ERR(*req->lower_file))
>>  		rc = PTR_ERR(*req->lower_file);
>> -		dget(lower_dentry);
>> -		mntget(lower_mnt);
>> -		(*lower_file) = dentry_open(lower_dentry, lower_mnt,
>> -					    (O_RDONLY | O_LARGEFILE), cred);
>> -		if (IS_ERR(*lower_file)) {
>> -			rc = PTR_ERR(*req->lower_file);
>> -			(*lower_file) = NULL;
>> -			printk(KERN_WARNING "%s: Error attempting privileged "
>> -			       "open of lower file with either RW or RO "
>> -			       "perms; rc = [%d]. Giving up.\n",
>> -			       __func__, rc);
>> -		}
>> -	}
>>  out_unlock:
>>  	mutex_unlock(&req->mux);
>>  out_free:
>> -- 
>> 1.6.2.5