| Message ID | 1482186016-107643-3-git-send-email-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 19, 2016 at 02:20:14PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Now that fscrypt_has_permitted_context() compares the fscrypt_context > rather than the fscrypt_info when needed, it is no longer necessary to > delay fscrypt_has_permitted_context() from ->lookup() to ->open() for > regular files, as introduced in commit ff978b09f973 ("ext4 crypto: move > context consistency check to ext4_file_open()"). Therefore the check in > ->open(), along with the dget_parent() hack, can be removed. It's also > no longer necessary to check the file type before calling > fscrypt_has_permitted_context(). There's a downside to this change. The change in the earlier commit of this series teaches fscrypt_has_permitted_context() can fall back to comparing the fscrypt_context. That's all very well and good, but it means that if you do a ls -l of an encrypted directory, and the key is not present, we will have to do an xattr lookup for every file in that directory. Even if the key is present, it will force the derivation of the per-file key of every file in that directory, regardless of whether the file is opened or not. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 28, 2016 at 12:41:02AM -0500, Theodore Ts'o wrote: > On Mon, Dec 19, 2016 at 02:20:14PM -0800, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > Now that fscrypt_has_permitted_context() compares the fscrypt_context > > rather than the fscrypt_info when needed, it is no longer necessary to > > delay fscrypt_has_permitted_context() from ->lookup() to ->open() for > > regular files, as introduced in commit ff978b09f973 ("ext4 crypto: move > > context consistency check to ext4_file_open()"). Therefore the check in > > ->open(), along with the dget_parent() hack, can be removed. It's also > > no longer necessary to check the file type before calling > > fscrypt_has_permitted_context(). > > There's a downside to this change. The change in the earlier commit > of this series teaches fscrypt_has_permitted_context() can fall back > to comparing the fscrypt_context. That's all very well and good, but > it means that if you do a ls -l of an encrypted directory, and the key > is not present, we will have to do an xattr lookup for every file in > that directory. Even if the key is present, it will force the > derivation of the per-file key of every file in that directory, > regardless of whether the file is opened or not. > > - Ted Hmm, I didn't know that was an intentional optimization. It would be nice to have a comment explaining it. It's not done on directories and symlinks --- is that because regular files require opening them to do anything with them, whereas without opening a directory you can create/mkdir/mknod/rmdir/etc., or without opening a symlink you can follow it? I wonder how sys_truncate() behaves; that doesn't require opening the file... Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/file.c b/fs/ext4/file.c index b5f1844..2123cd8 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -398,7 +398,6 @@ static int ext4_file_open(struct inode * inode, struct file * filp) struct super_block *sb = inode->i_sb; struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb); struct vfsmount *mnt = filp->f_path.mnt; - struct dentry *dir; struct path path; char buf[64], *cp; int ret; @@ -443,17 +442,6 @@ static int ext4_file_open(struct inode * inode, struct file * filp) return -ENOKEY; } - dir = dget_parent(file_dentry(filp)); - if (ext4_encrypted_inode(d_inode(dir)) && - !fscrypt_has_permitted_context(d_inode(dir), inode)) { - ext4_warning(inode->i_sb, - "Inconsistent encryption contexts: %lu/%lu", - (unsigned long) d_inode(dir)->i_ino, - (unsigned long) inode->i_ino); - dput(dir); - return -EPERM; - } - dput(dir); /* * Set up the jbd2_inode if we are opening the inode for * writing and the journal is present diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index eadba91..eb8b064 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1612,17 +1612,11 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi return ERR_PTR(-EFSCORRUPTED); } if (!IS_ERR(inode) && ext4_encrypted_inode(dir) && - (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) && !fscrypt_has_permitted_context(dir, inode)) { - int nokey = ext4_encrypted_inode(inode) && - !fscrypt_has_encryption_key(inode); - iput(inode); - if (nokey) - return ERR_PTR(-ENOKEY); ext4_warning(inode->i_sb, "Inconsistent encryption contexts: %lu/%lu", - (unsigned long) dir->i_ino, - (unsigned long) inode->i_ino); + dir->i_ino, inode->i_ino); + iput(inode); return ERR_PTR(-EPERM); } }