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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
