| Message ID | 20200904160537.76663-13-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ceph+fscrypt: context, filename and symlink support | expand |
On Fri, Sep 04, 2020 at 12:05:31PM -0400, Jeff Layton wrote: > This hack fixes a chicken-and-egg problem when fetching inodes from the > server. Once we move to a dedicated field in the inode, then this should > be able to go away. To clarify: while this *could* be the permanent solution, you're planning to make ceph support storing an "is inode encrypted?" flag on the server, similar to what the local filesystems do with i_flags (since searching the xattrs for every inode is much more expensive than a simple flag check)? > +#define DUMMY_ENCRYPTION_ENABLED(fsc) ((fsc)->dummy_enc_ctx.ctx != NULL) > + As I mentioned on an earlier patch, please put the support for the "test_dummy_encryption" mount option in a separate patch. It's best thought of separately from the basic fscrypt support. > int ceph_fscrypt_set_ops(struct super_block *sb); > int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, > struct ceph_acl_sec_ctx *as); > > #else /* CONFIG_FS_ENCRYPTION */ > > +#define DUMMY_ENCRYPTION_ENABLED(fsc) (0) > + > static inline int ceph_fscrypt_set_ops(struct super_block *sb) > { > return 0; > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 651148194316..c1c1fe2547f9 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -964,6 +964,10 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page, > ceph_forget_all_cached_acls(inode); > ceph_security_invalidate_secctx(inode); > xattr_blob = NULL; > + if ((inode->i_state & I_NEW) && > + (DUMMY_ENCRYPTION_ENABLED(mdsc->fsc) || > + ceph_inode_has_xattr(ci, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT))) > + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED); The check for DUMMY_ENCRYPTION_ENABLED() here is wrong and should be removed. When the filesystem is mounted with test_dummy_encryption, there may be unencrypted inodes already on-disk. Those shouldn't get S_ENCRYPTED set. test_dummy_encryption does add some special handling for unencrypted directories, but that doesn't require S_ENCRYPTED to be set on them. - Eric
On Mon, 2020-09-07 at 21:57 -0700, Eric Biggers wrote: > On Fri, Sep 04, 2020 at 12:05:31PM -0400, Jeff Layton wrote: > > This hack fixes a chicken-and-egg problem when fetching inodes from the > > server. Once we move to a dedicated field in the inode, then this should > > be able to go away. > > To clarify: while this *could* be the permanent solution, you're planning to > make ceph support storing an "is inode encrypted?" flag on the server, similar > to what the local filesystems do with i_flags (since searching the xattrs for > every inode is much more expensive than a simple flag check)? > That was what I was thinking before, but now it's looking like we'll need to keep this in the xattrs. So I may still need this hack in perpetuity. > > +#define DUMMY_ENCRYPTION_ENABLED(fsc) ((fsc)->dummy_enc_ctx.ctx != NULL) > > + > > As I mentioned on an earlier patch, please put the support for the > "test_dummy_encryption" mount option in a separate patch. It's best thought of > separately from the basic fscrypt support. > Yep. The next series will have that in a separate patch. > > int ceph_fscrypt_set_ops(struct super_block *sb); > > int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, > > struct ceph_acl_sec_ctx *as); > > > > #else /* CONFIG_FS_ENCRYPTION */ > > > > +#define DUMMY_ENCRYPTION_ENABLED(fsc) (0) > > + > > static inline int ceph_fscrypt_set_ops(struct super_block *sb) > > { > > return 0; > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > index 651148194316..c1c1fe2547f9 100644 > > --- a/fs/ceph/inode.c > > +++ b/fs/ceph/inode.c > > @@ -964,6 +964,10 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page, > > ceph_forget_all_cached_acls(inode); > > ceph_security_invalidate_secctx(inode); > > xattr_blob = NULL; > > + if ((inode->i_state & I_NEW) && > > + (DUMMY_ENCRYPTION_ENABLED(mdsc->fsc) || > > + ceph_inode_has_xattr(ci, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT))) > > + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED); > > The check for DUMMY_ENCRYPTION_ENABLED() here is wrong and should be removed. > When the filesystem is mounted with test_dummy_encryption, there may be > unencrypted inodes already on-disk. Those shouldn't get S_ENCRYPTED set. > test_dummy_encryption does add some special handling for unencrypted > directories, but that doesn't require S_ENCRYPTED to be set on them. > Got it, thanks. The fact that you still need to keep a context when you enable dummy encryption wasn't clear to me before.
On Mon, 2020-09-07 at 21:57 -0700, Eric Biggers wrote: > On Fri, Sep 04, 2020 at 12:05:31PM -0400, Jeff Layton wrote: > > This hack fixes a chicken-and-egg problem when fetching inodes from the > > server. Once we move to a dedicated field in the inode, then this should > > be able to go away. > > To clarify: while this *could* be the permanent solution, you're planning to > make ceph support storing an "is inode encrypted?" flag on the server, similar > to what the local filesystems do with i_flags (since searching the xattrs for > every inode is much more expensive than a simple flag check)? > > > +#define DUMMY_ENCRYPTION_ENABLED(fsc) ((fsc)->dummy_enc_ctx.ctx != NULL) > > + > > As I mentioned on an earlier patch, please put the support for the > "test_dummy_encryption" mount option in a separate patch. It's best thought of > separately from the basic fscrypt support. > > > int ceph_fscrypt_set_ops(struct super_block *sb); > > int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, > > struct ceph_acl_sec_ctx *as); > > > > #else /* CONFIG_FS_ENCRYPTION */ > > > > +#define DUMMY_ENCRYPTION_ENABLED(fsc) (0) > > + > > static inline int ceph_fscrypt_set_ops(struct super_block *sb) > > { > > return 0; > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > index 651148194316..c1c1fe2547f9 100644 > > --- a/fs/ceph/inode.c > > +++ b/fs/ceph/inode.c > > @@ -964,6 +964,10 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page, > > ceph_forget_all_cached_acls(inode); > > ceph_security_invalidate_secctx(inode); > > xattr_blob = NULL; > > + if ((inode->i_state & I_NEW) && > > + (DUMMY_ENCRYPTION_ENABLED(mdsc->fsc) || > > + ceph_inode_has_xattr(ci, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT))) > > + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED); > > The check for DUMMY_ENCRYPTION_ENABLED() here is wrong and should be removed. > When the filesystem is mounted with test_dummy_encryption, there may be > unencrypted inodes already on-disk. Those shouldn't get S_ENCRYPTED set. > test_dummy_encryption does add some special handling for unencrypted > directories, but that doesn't require S_ENCRYPTED to be set on them. > I've been working through your comments. Symlinks work now, as long as I use the fscrypt utility instead of test_dummy_encryption. When I remove that condition above, then test_dummy_encryption no longer works. I think I must be missing some context in how test_dummy_encryption is supposed to be used. Suppose I mount a ceph filesystem with -o test_dummy_encryption. The root inode of the fs is instantiated by going through here, but it's not marked with S_ENCRYPTED because the root has no context. How will descendant dentries end up encrypted if this one is not marked as such? Thanks,
On Wed, Sep 09, 2020 at 11:53:35AM -0400, Jeff Layton wrote: > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > > index 651148194316..c1c1fe2547f9 100644 > > > --- a/fs/ceph/inode.c > > > +++ b/fs/ceph/inode.c > > > @@ -964,6 +964,10 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page, > > > ceph_forget_all_cached_acls(inode); > > > ceph_security_invalidate_secctx(inode); > > > xattr_blob = NULL; > > > + if ((inode->i_state & I_NEW) && > > > + (DUMMY_ENCRYPTION_ENABLED(mdsc->fsc) || > > > + ceph_inode_has_xattr(ci, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT))) > > > + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED); > > > > The check for DUMMY_ENCRYPTION_ENABLED() here is wrong and should be removed. > > When the filesystem is mounted with test_dummy_encryption, there may be > > unencrypted inodes already on-disk. Those shouldn't get S_ENCRYPTED set. > > test_dummy_encryption does add some special handling for unencrypted > > directories, but that doesn't require S_ENCRYPTED to be set on them. > > > > I've been working through your comments. Symlinks work now, as long as I > use the fscrypt utility instead of test_dummy_encryption. > > When I remove that condition above, then test_dummy_encryption no longer > works. I think I must be missing some context in how > test_dummy_encryption is supposed to be used. > > Suppose I mount a ceph filesystem with -o test_dummy_encryption. The > root inode of the fs is instantiated by going through here, but it's not > marked with S_ENCRYPTED because the root has no context. > > How will descendant dentries end up encrypted if this one is not marked > as such? See fscrypt_prepare_new_inode() (or in current mainline, the logic in __ext4_new_inode() and f2fs_may_encrypt()). Encryption gets inherited if: IS_ENCRYPTED(dir) || fscrypt_get_dummy_context(dir->i_sb) != NULL instead of just: IS_ENCRYPTED(dir) Note that this specifically refers to encryption being *inherited*. If !IS_ENCRYPTED(dir), then the directory itself remains unencrypted --- that means that the filenames are in the directory aren't encrypted, even if they name encrypted files/directories/symlinks. - Eric
On Wed, 2020-09-09 at 09:33 -0700, Eric Biggers wrote: > On Wed, Sep 09, 2020 at 11:53:35AM -0400, Jeff Layton wrote: > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > > > index 651148194316..c1c1fe2547f9 100644 > > > > --- a/fs/ceph/inode.c > > > > +++ b/fs/ceph/inode.c > > > > @@ -964,6 +964,10 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page, > > > > ceph_forget_all_cached_acls(inode); > > > > ceph_security_invalidate_secctx(inode); > > > > xattr_blob = NULL; > > > > + if ((inode->i_state & I_NEW) && > > > > + (DUMMY_ENCRYPTION_ENABLED(mdsc->fsc) || > > > > + ceph_inode_has_xattr(ci, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT))) > > > > + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED); > > > > > > The check for DUMMY_ENCRYPTION_ENABLED() here is wrong and should be removed. > > > When the filesystem is mounted with test_dummy_encryption, there may be > > > unencrypted inodes already on-disk. Those shouldn't get S_ENCRYPTED set. > > > test_dummy_encryption does add some special handling for unencrypted > > > directories, but that doesn't require S_ENCRYPTED to be set on them. > > > > > > > I've been working through your comments. Symlinks work now, as long as I > > use the fscrypt utility instead of test_dummy_encryption. > > > > When I remove that condition above, then test_dummy_encryption no longer > > works. I think I must be missing some context in how > > test_dummy_encryption is supposed to be used. > > > > Suppose I mount a ceph filesystem with -o test_dummy_encryption. The > > root inode of the fs is instantiated by going through here, but it's not > > marked with S_ENCRYPTED because the root has no context. > > > > How will descendant dentries end up encrypted if this one is not marked > > as such? > > See fscrypt_prepare_new_inode() (or in current mainline, the logic in > __ext4_new_inode() and f2fs_may_encrypt()). Encryption gets inherited if: > > IS_ENCRYPTED(dir) || fscrypt_get_dummy_context(dir->i_sb) != NULL > > instead of just: > > IS_ENCRYPTED(dir) > > Note that this specifically refers to encryption being *inherited*. > If !IS_ENCRYPTED(dir), then the directory itself remains unencrypted --- > that means that the filenames are in the directory aren't encrypted, even if > they name encrypted files/directories/symlinks. > Ok, I think I get it, but it is a little weird. I would have thought that test_dummy_encryption would just replace the usage of whatever's in the xattr with the per-sb one (or maybe only when one doesn't exist). This is a bit different though, in that the dummy context only comes into play with newly created inodes. So, if I mount an empty subdirectory using test_dummy_encryption, you can't encrypt the names at the root. I guess that's sort of like how it works with "regular" contexts too, and that prevents someone clobbering old data mistakenly. Thanks for the explanation!
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h index bf893bd215c3..1b11e9af165e 100644 --- a/fs/ceph/crypto.h +++ b/fs/ceph/crypto.h @@ -10,12 +10,16 @@ #define CEPH_XATTR_NAME_ENCRYPTION_CONTEXT "encryption.ctx" +#define DUMMY_ENCRYPTION_ENABLED(fsc) ((fsc)->dummy_enc_ctx.ctx != NULL) + int ceph_fscrypt_set_ops(struct super_block *sb); int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, struct ceph_acl_sec_ctx *as); #else /* CONFIG_FS_ENCRYPTION */ +#define DUMMY_ENCRYPTION_ENABLED(fsc) (0) + static inline int ceph_fscrypt_set_ops(struct super_block *sb) { return 0; diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 651148194316..c1c1fe2547f9 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -964,6 +964,10 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page, ceph_forget_all_cached_acls(inode); ceph_security_invalidate_secctx(inode); xattr_blob = NULL; + if ((inode->i_state & I_NEW) && + (DUMMY_ENCRYPTION_ENABLED(mdsc->fsc) || + ceph_inode_has_xattr(ci, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT))) + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED); } /* finally update i_version */ diff --git a/fs/ceph/super.h b/fs/ceph/super.h index cf04fcd3de69..7c859824f64c 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -986,6 +986,7 @@ extern ssize_t ceph_listxattr(struct dentry *, char *, size_t); extern struct ceph_buffer *__ceph_build_xattrs_blob(struct ceph_inode_info *ci); extern void __ceph_destroy_xattrs(struct ceph_inode_info *ci); extern const struct xattr_handler *ceph_xattr_handlers[]; +bool ceph_inode_has_xattr(struct ceph_inode_info *ci, char *name); struct ceph_acl_sec_ctx { #ifdef CONFIG_CEPH_FS_POSIX_ACL diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c index 3a733ac33d9b..9dcb060cba9a 100644 --- a/fs/ceph/xattr.c +++ b/fs/ceph/xattr.c @@ -1283,6 +1283,38 @@ void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx) ceph_pagelist_release(as_ctx->pagelist); } +/* Return true if inode's xattr blob has an xattr named "name" */ +bool ceph_inode_has_xattr(struct ceph_inode_info *ci, char *name) +{ + void *p, *end; + u32 numattr; + size_t namelen; + + lockdep_assert_held(&ci->i_ceph_lock); + + if (!ci->i_xattrs.blob || ci->i_xattrs.blob->vec.iov_len <= 4) + return false; + + namelen = strlen(name); + p = ci->i_xattrs.blob->vec.iov_base; + end = p + ci->i_xattrs.blob->vec.iov_len; + ceph_decode_32_safe(&p, end, numattr, bad); + + while (numattr--) { + u32 len; + + ceph_decode_32_safe(&p, end, len, bad); + ceph_decode_need(&p, end, len, bad); + if (len == namelen && !memcmp(p, name, len)) + return true; + p += len; + ceph_decode_32_safe(&p, end, len, bad); + ceph_decode_skip_n(&p, end, len, bad); + } +bad: + return false; +} + /* * List of handlers for synthetic system.* attributes. Other * attributes are handled directly.
This hack fixes a chicken-and-egg problem when fetching inodes from the server. Once we move to a dedicated field in the inode, then this should be able to go away. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/crypto.h | 4 ++++ fs/ceph/inode.c | 4 ++++ fs/ceph/super.h | 1 + fs/ceph/xattr.c | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 41 insertions(+)