On 09/18/2009 04:50 PM, Serge E. Hallyn wrote: > Quoting Tyler Hicks ([email protected]): >> 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 <[email protected]> >> Cc: Eric Sandeen <[email protected]> >> Cc: Dave Kleikamp <[email protected]> >> Cc: [email protected] >> Signed-off-by: Tyler Hicks <[email protected]> > > 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 <[email protected]> > > 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 _______________________________________________ Mailing list: https://launchpad.net/~ecryptfs-devel Post to : [email protected] Unsubscribe : https://launchpad.net/~ecryptfs-devel More help : https://help.launchpad.net/ListHelp
