diff mbox series

[RFC,v2,12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr

Message ID 20200904160537.76663-13-jlayton@kernel.org
State Superseded
Headers show
Series ceph+fscrypt: context, filename and symlink support | expand

Commit Message

Jeff Layton Sept. 4, 2020, 4:05 p.m. UTC
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(+)

Comments

Eric Biggers Sept. 8, 2020, 4:57 a.m. UTC | #1
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
Jeff Layton Sept. 9, 2020, 12:20 p.m. UTC | #2
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 Sept. 9, 2020, 3:53 p.m. UTC | #3
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,
Eric Biggers Sept. 9, 2020, 4:33 p.m. UTC | #4
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
Jeff Layton Sept. 9, 2020, 5:19 p.m. UTC | #5
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 mbox series

Patch

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.