diff mbox series

[RFC,v4,05/17] ceph: crypto context handling for ceph

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

Commit Message

Jeff Layton Jan. 20, 2021, 6:28 p.m. UTC 
Store the fscrypt context for an inode as an encryption.ctx xattr.
When we get a new inode in a trace, set the S_ENCRYPTED bit if
the xattr blob has an encryption.ctx xattr.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/Makefile |  1 +
 fs/ceph/crypto.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 fs/ceph/crypto.h | 24 ++++++++++++++++++++++++
 fs/ceph/inode.c  |  6 ++++++
 fs/ceph/super.c  |  3 +++
 fs/ceph/super.h  |  1 +
 fs/ceph/xattr.c  | 32 ++++++++++++++++++++++++++++++++
 7 files changed, 109 insertions(+)
 create mode 100644 fs/ceph/crypto.c
 create mode 100644 fs/ceph/crypto.h

Comments

Luis Henriques Jan. 22, 2021, 4:41 p.m. UTC | #1 
Jeff Layton <jlayton@kernel.org> writes:

> Store the fscrypt context for an inode as an encryption.ctx xattr.

> When we get a new inode in a trace, set the S_ENCRYPTED bit if

> the xattr blob has an encryption.ctx xattr.

>

> Signed-off-by: Jeff Layton <jlayton@kernel.org>

> ---

>  fs/ceph/Makefile |  1 +

>  fs/ceph/crypto.c | 42 ++++++++++++++++++++++++++++++++++++++++++

>  fs/ceph/crypto.h | 24 ++++++++++++++++++++++++

>  fs/ceph/inode.c  |  6 ++++++

>  fs/ceph/super.c  |  3 +++

>  fs/ceph/super.h  |  1 +

>  fs/ceph/xattr.c  | 32 ++++++++++++++++++++++++++++++++

>  7 files changed, 109 insertions(+)

>  create mode 100644 fs/ceph/crypto.c

>  create mode 100644 fs/ceph/crypto.h

>

> diff --git a/fs/ceph/Makefile b/fs/ceph/Makefile

> index 50c635dc7f71..1f77ca04c426 100644

> --- a/fs/ceph/Makefile

> +++ b/fs/ceph/Makefile

> @@ -12,3 +12,4 @@ ceph-y := super.o inode.o dir.o file.o locks.o addr.o ioctl.o \

>  

>  ceph-$(CONFIG_CEPH_FSCACHE) += cache.o

>  ceph-$(CONFIG_CEPH_FS_POSIX_ACL) += acl.o

> +ceph-$(CONFIG_FS_ENCRYPTION) += crypto.o

> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c

> new file mode 100644

> index 000000000000..dbe8b60fd1b0

> --- /dev/null

> +++ b/fs/ceph/crypto.c

> @@ -0,0 +1,42 @@

> +// SPDX-License-Identifier: GPL-2.0

> +#include <linux/ceph/ceph_debug.h>

> +#include <linux/xattr.h>

> +#include <linux/fscrypt.h>

> +

> +#include "super.h"

> +#include "crypto.h"

> +

> +static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)

> +{

> +	return __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);

> +}

> +

> +static int ceph_crypt_set_context(struct inode *inode, const void *ctx, size_t len, void *fs_data)

> +{

> +	int ret;

> +

> +	WARN_ON_ONCE(fs_data);

> +	ret = __ceph_setxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len, XATTR_CREATE);

> +	if (ret == 0)

> +		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);

> +	return ret;

> +}

> +

> +static bool ceph_crypt_empty_dir(struct inode *inode)

> +{

> +	struct ceph_inode_info *ci = ceph_inode(inode);

> +

> +	return ci->i_rsubdirs + ci->i_rfiles == 1;

> +}


This is very tricky, as this check can't really guaranty that the
directory is empty.  We need to make sure no other client has access to
this directory during the whole operation of setting policy.  Would it be
enough to ensure we have Fxc here?

> +

> +static struct fscrypt_operations ceph_fscrypt_ops = {

> +	.get_context		= ceph_crypt_get_context,

> +	.set_context		= ceph_crypt_set_context,

> +	.empty_dir		= ceph_crypt_empty_dir,

> +	.max_namelen		= NAME_MAX,

> +};

> +

> +void ceph_fscrypt_set_ops(struct super_block *sb)

> +{

> +	fscrypt_set_ops(sb, &ceph_fscrypt_ops);

> +}

> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h

> new file mode 100644

> index 000000000000..189bd8424284

> --- /dev/null

> +++ b/fs/ceph/crypto.h

> @@ -0,0 +1,24 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * Ceph fscrypt functionality

> + */

> +

> +#ifndef _CEPH_CRYPTO_H

> +#define _CEPH_CRYPTO_H

> +

> +#include <linux/fscrypt.h>

> +

> +#define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"

> +

> +#ifdef CONFIG_FS_ENCRYPTION

> +void ceph_fscrypt_set_ops(struct super_block *sb);

> +

> +#else /* CONFIG_FS_ENCRYPTION */

> +

> +static inline void ceph_fscrypt_set_ops(struct super_block *sb)

> +{

> +}

> +

> +#endif /* CONFIG_FS_ENCRYPTION */

> +

> +#endif

> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c

> index 5d20a620e96c..d465ad48ade5 100644

> --- a/fs/ceph/inode.c

> +++ b/fs/ceph/inode.c

> @@ -14,10 +14,12 @@

>  #include <linux/random.h>

>  #include <linux/sort.h>

>  #include <linux/iversion.h>

> +#include <linux/fscrypt.h>

>  

>  #include "super.h"

>  #include "mds_client.h"

>  #include "cache.h"

> +#include "crypto.h"

>  #include <linux/ceph/decode.h>

>  

>  /*

> @@ -553,6 +555,7 @@ void ceph_evict_inode(struct inode *inode)

>  	clear_inode(inode);

>  

>  	ceph_fscache_unregister_inode_cookie(ci);

> +	fscrypt_put_encryption_info(inode);

>  

>  	__ceph_remove_caps(ci);

>  

> @@ -912,6 +915,9 @@ 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) &&

> +		     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.c b/fs/ceph/super.c

> index 9b1b7f4cfdd4..cdac6ff675e2 100644

> --- a/fs/ceph/super.c

> +++ b/fs/ceph/super.c

> @@ -20,6 +20,7 @@

>  #include "super.h"

>  #include "mds_client.h"

>  #include "cache.h"

> +#include "crypto.h"

>  

>  #include <linux/ceph/ceph_features.h>

>  #include <linux/ceph/decode.h>

> @@ -988,6 +989,8 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)

>  	s->s_time_min = 0;

>  	s->s_time_max = U32_MAX;

>  

> +	ceph_fscrypt_set_ops(s);

> +

>  	ret = set_anon_super_fc(s, fc);

>  	if (ret != 0)

>  		fsc->sb = NULL;

> diff --git a/fs/ceph/super.h b/fs/ceph/super.h

> index 13b02887b085..efe2e963c631 100644

> --- a/fs/ceph/super.h

> +++ b/fs/ceph/super.h

> @@ -1013,6 +1013,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, const 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 24997982de01..d0d719b768e4 100644

> --- a/fs/ceph/xattr.c

> +++ b/fs/ceph/xattr.c

> @@ -1359,6 +1359,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, const 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;

> +}


I wonder if it wouldn't be better have an extra flag in struct
ceph_inode_info instead of having to go through the xattr list every time
we update an inode with data from the MDS.

> 

> +

>  /*

>   * List of handlers for synthetic system.* attributes. Other

>   * attributes are handled directly.

> -- 

>

> 2.29.2

>


-- 
Luis
Jeff Layton Jan. 22, 2021, 5:26 p.m. UTC | #2 
On Fri, 2021-01-22 at 16:41 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:

> 

> > Store the fscrypt context for an inode as an encryption.ctx xattr.

> > When we get a new inode in a trace, set the S_ENCRYPTED bit if

> > the xattr blob has an encryption.ctx xattr.

> > 

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>

> > ---

> >  fs/ceph/Makefile |  1 +

> >  fs/ceph/crypto.c | 42 ++++++++++++++++++++++++++++++++++++++++++

> >  fs/ceph/crypto.h | 24 ++++++++++++++++++++++++

> >  fs/ceph/inode.c  |  6 ++++++

> >  fs/ceph/super.c  |  3 +++

> >  fs/ceph/super.h  |  1 +

> >  fs/ceph/xattr.c  | 32 ++++++++++++++++++++++++++++++++

> >  7 files changed, 109 insertions(+)

> >  create mode 100644 fs/ceph/crypto.c

> >  create mode 100644 fs/ceph/crypto.h

> > 

> > diff --git a/fs/ceph/Makefile b/fs/ceph/Makefile

> > index 50c635dc7f71..1f77ca04c426 100644

> > --- a/fs/ceph/Makefile

> > +++ b/fs/ceph/Makefile

> > @@ -12,3 +12,4 @@ ceph-y := super.o inode.o dir.o file.o locks.o addr.o ioctl.o \

> >  

> > 

> > 

> > 

> >  ceph-$(CONFIG_CEPH_FSCACHE) += cache.o

> >  ceph-$(CONFIG_CEPH_FS_POSIX_ACL) += acl.o

> > +ceph-$(CONFIG_FS_ENCRYPTION) += crypto.o

> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c

> > new file mode 100644

> > index 000000000000..dbe8b60fd1b0

> > --- /dev/null

> > +++ b/fs/ceph/crypto.c

> > @@ -0,0 +1,42 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +#include <linux/ceph/ceph_debug.h>

> > +#include <linux/xattr.h>

> > +#include <linux/fscrypt.h>

> > +

> > +#include "super.h"

> > +#include "crypto.h"

> > +

> > +static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)

> > +{

> > +	return __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);

> > +}

> > +

> > +static int ceph_crypt_set_context(struct inode *inode, const void *ctx, size_t len, void *fs_data)

> > +{

> > +	int ret;

> > +

> > +	WARN_ON_ONCE(fs_data);

> > +	ret = __ceph_setxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len, XATTR_CREATE);

> > +	if (ret == 0)

> > +		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);

> > +	return ret;

> > +}

> > +

> > +static bool ceph_crypt_empty_dir(struct inode *inode)

> > +{

> > +	struct ceph_inode_info *ci = ceph_inode(inode);

> > +

> > +	return ci->i_rsubdirs + ci->i_rfiles == 1;

> > +}

> 

> This is very tricky, as this check can't really guaranty that the

> directory is empty.  We need to make sure no other client has access to

> this directory during the whole operation of setting policy.  Would it be

> enough to ensure we have Fxc here?

> 


That's a good point. Yes, we probably should do just that. I'll take a
look and see what we need as far as caps go to ensure exclusion.

empty_dir is only called when setting the context, so we can grab cap
refs at that point and then just check to make sure it's empty here.

> > +

> > +static struct fscrypt_operations ceph_fscrypt_ops = {

> > +	.get_context		= ceph_crypt_get_context,

> > +	.set_context		= ceph_crypt_set_context,

> > +	.empty_dir		= ceph_crypt_empty_dir,

> > +	.max_namelen		= NAME_MAX,

> > +};

> > +

> > +void ceph_fscrypt_set_ops(struct super_block *sb)

> > +{

> > +	fscrypt_set_ops(sb, &ceph_fscrypt_ops);

> > +}

> > diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h

> > new file mode 100644

> > index 000000000000..189bd8424284

> > --- /dev/null

> > +++ b/fs/ceph/crypto.h

> > @@ -0,0 +1,24 @@

> > +/* SPDX-License-Identifier: GPL-2.0 */

> > +/*

> > + * Ceph fscrypt functionality

> > + */

> > +

> > +#ifndef _CEPH_CRYPTO_H

> > +#define _CEPH_CRYPTO_H

> > +

> > +#include <linux/fscrypt.h>

> > +

> > +#define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"

> > +

> > +#ifdef CONFIG_FS_ENCRYPTION

> > +void ceph_fscrypt_set_ops(struct super_block *sb);

> > +

> > +#else /* CONFIG_FS_ENCRYPTION */

> > +

> > +static inline void ceph_fscrypt_set_ops(struct super_block *sb)

> > +{

> > +}

> > +

> > +#endif /* CONFIG_FS_ENCRYPTION */

> > +

> > +#endif

> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c

> > index 5d20a620e96c..d465ad48ade5 100644

> > --- a/fs/ceph/inode.c

> > +++ b/fs/ceph/inode.c

> > @@ -14,10 +14,12 @@

> >  #include <linux/random.h>

> >  #include <linux/sort.h>

> >  #include <linux/iversion.h>

> > +#include <linux/fscrypt.h>

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> >  #include "super.h"

> >  #include "mds_client.h"

> >  #include "cache.h"

> > +#include "crypto.h"

> >  #include <linux/ceph/decode.h>

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> >  /*

> > @@ -553,6 +555,7 @@ void ceph_evict_inode(struct inode *inode)

> >  	clear_inode(inode);

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> >  	ceph_fscache_unregister_inode_cookie(ci);

> > +	fscrypt_put_encryption_info(inode);

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> >  	__ceph_remove_caps(ci);

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> > @@ -912,6 +915,9 @@ 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) &&

> > +		     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.c b/fs/ceph/super.c

> > index 9b1b7f4cfdd4..cdac6ff675e2 100644

> > --- a/fs/ceph/super.c

> > +++ b/fs/ceph/super.c

> > @@ -20,6 +20,7 @@

> >  #include "super.h"

> >  #include "mds_client.h"

> >  #include "cache.h"

> > +#include "crypto.h"

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> >  #include <linux/ceph/ceph_features.h>

> >  #include <linux/ceph/decode.h>

> > @@ -988,6 +989,8 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)

> >  	s->s_time_min = 0;

> >  	s->s_time_max = U32_MAX;

> >  

> > 

> > 

> > 

> > 

> > 

> > 

> > 

> > +	ceph_fscrypt_set_ops(s);

> > +

> >  	ret = set_anon_super_fc(s, fc);

> >  	if (ret != 0)

> >  		fsc->sb = NULL;

> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h

> > index 13b02887b085..efe2e963c631 100644

> > --- a/fs/ceph/super.h

> > +++ b/fs/ceph/super.h

> > @@ -1013,6 +1013,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, const 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 24997982de01..d0d719b768e4 100644

> > --- a/fs/ceph/xattr.c

> > +++ b/fs/ceph/xattr.c

> > @@ -1359,6 +1359,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, const 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;

> > +}

> 

> I wonder if it wouldn't be better have an extra flag in struct

> ceph_inode_info instead of having to go through the xattr list every time

> we update an inode with data from the MDS.

> 


It is ugly, I'll grant you that. A flag in ceph_inode_info won't really
help though. We'd need some sort of flag in the inode info transmitted
from the MDS.

For now, we only call this for I_NEW inodes, but now I'm wondering if we
need to check it every time, to ensure that everything works if you see
the directory before another client encrypts it.

We may want to extend the protocol with such a flag, but the xattr
buffer is not usually that long. For now, I'm inclined to live with
parsing this info each time.

> > 

> > +

> >  /*

> >   * List of handlers for synthetic system.* attributes. Other

> >   * attributes are handled directly.

> > -- 

> > 

> > 2.29.2

> > 

> 


-- 
Jeff Layton <jlayton@kernel.org>
Luis Henriques Jan. 25, 2021, 10:14 a.m. UTC | #3 
Jeff Layton <jlayton@kernel.org> writes:

> On Fri, 2021-01-22 at 16:41 +0000, Luis Henriques wrote:

>> Jeff Layton <jlayton@kernel.org> writes:

>> 

>> > Store the fscrypt context for an inode as an encryption.ctx xattr.

>> > When we get a new inode in a trace, set the S_ENCRYPTED bit if

>> > the xattr blob has an encryption.ctx xattr.

>> > 

>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>

>> > ---

>> >  fs/ceph/Makefile |  1 +

>> >  fs/ceph/crypto.c | 42 ++++++++++++++++++++++++++++++++++++++++++

>> >  fs/ceph/crypto.h | 24 ++++++++++++++++++++++++

>> >  fs/ceph/inode.c  |  6 ++++++

>> >  fs/ceph/super.c  |  3 +++

>> >  fs/ceph/super.h  |  1 +

>> >  fs/ceph/xattr.c  | 32 ++++++++++++++++++++++++++++++++

>> >  7 files changed, 109 insertions(+)

>> >  create mode 100644 fs/ceph/crypto.c

>> >  create mode 100644 fs/ceph/crypto.h

>> > 

>> > diff --git a/fs/ceph/Makefile b/fs/ceph/Makefile

>> > index 50c635dc7f71..1f77ca04c426 100644

>> > --- a/fs/ceph/Makefile

>> > +++ b/fs/ceph/Makefile

>> > @@ -12,3 +12,4 @@ ceph-y := super.o inode.o dir.o file.o locks.o addr.o ioctl.o \

>> >  

>> > 

>> > 

>> > 

>> >  ceph-$(CONFIG_CEPH_FSCACHE) += cache.o

>> >  ceph-$(CONFIG_CEPH_FS_POSIX_ACL) += acl.o

>> > +ceph-$(CONFIG_FS_ENCRYPTION) += crypto.o

>> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c

>> > new file mode 100644

>> > index 000000000000..dbe8b60fd1b0

>> > --- /dev/null

>> > +++ b/fs/ceph/crypto.c

>> > @@ -0,0 +1,42 @@

>> > +// SPDX-License-Identifier: GPL-2.0

>> > +#include <linux/ceph/ceph_debug.h>

>> > +#include <linux/xattr.h>

>> > +#include <linux/fscrypt.h>

>> > +

>> > +#include "super.h"

>> > +#include "crypto.h"

>> > +

>> > +static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)

>> > +{

>> > +	return __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);

>> > +}

>> > +

>> > +static int ceph_crypt_set_context(struct inode *inode, const void *ctx, size_t len, void *fs_data)

>> > +{

>> > +	int ret;

>> > +

>> > +	WARN_ON_ONCE(fs_data);

>> > +	ret = __ceph_setxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len, XATTR_CREATE);

>> > +	if (ret == 0)

>> > +		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);

>> > +	return ret;

>> > +}

>> > +

>> > +static bool ceph_crypt_empty_dir(struct inode *inode)

>> > +{

>> > +	struct ceph_inode_info *ci = ceph_inode(inode);

>> > +

>> > +	return ci->i_rsubdirs + ci->i_rfiles == 1;

>> > +}

>> 

>> This is very tricky, as this check can't really guaranty that the

>> directory is empty.  We need to make sure no other client has access to

>> this directory during the whole operation of setting policy.  Would it be

>> enough to ensure we have Fxc here?

>> 

>

> That's a good point. Yes, we probably should do just that. I'll take a

> look and see what we need as far as caps go to ensure exclusion.

>

> empty_dir is only called when setting the context, so we can grab cap

> refs at that point and then just check to make sure it's empty here.

>

>> > +

...
>> > +static struct fscrypt_operations ceph_fscrypt_ops = {

>> > +/* Return true if inode's xattr blob has an xattr named "name" */

>> > +bool ceph_inode_has_xattr(struct ceph_inode_info *ci, const 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;

>> > +}

>> 

>> I wonder if it wouldn't be better have an extra flag in struct

>> ceph_inode_info instead of having to go through the xattr list every time

>> we update an inode with data from the MDS.

>> 

>

> It is ugly, I'll grant you that. A flag in ceph_inode_info won't really

> help though. We'd need some sort of flag in the inode info transmitted

> from the MDS.

>

> For now, we only call this for I_NEW inodes, but now I'm wondering if we

> need to check it every time, to ensure that everything works if you see

> the directory before another client encrypts it.

>

> We may want to extend the protocol with such a flag, but the xattr

> buffer is not usually that long. For now, I'm inclined to live with

> parsing this info each time.


Ok, makes sense.  Thanks for clarifying.  I guess that getting the Fxc
caps when encrypting a dir (setting the context) may actually prevent the
race you mention anyway.

Cheers,
-- 
Luis
diff mbox series

Patch

diff --git a/fs/ceph/Makefile b/fs/ceph/Makefile
index 50c635dc7f71..1f77ca04c426 100644
--- a/fs/ceph/Makefile
+++ b/fs/ceph/Makefile
@@ -12,3 +12,4 @@  ceph-y := super.o inode.o dir.o file.o locks.o addr.o ioctl.o \
 
 ceph-$(CONFIG_CEPH_FSCACHE) += cache.o
 ceph-$(CONFIG_CEPH_FS_POSIX_ACL) += acl.o
+ceph-$(CONFIG_FS_ENCRYPTION) += crypto.o
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
new file mode 100644
index 000000000000..dbe8b60fd1b0
--- /dev/null
+++ b/fs/ceph/crypto.c
@@ -0,0 +1,42 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/ceph/ceph_debug.h>
+#include <linux/xattr.h>
+#include <linux/fscrypt.h>
+
+#include "super.h"
+#include "crypto.h"
+
+static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
+{
+	return __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);
+}
+
+static int ceph_crypt_set_context(struct inode *inode, const void *ctx, size_t len, void *fs_data)
+{
+	int ret;
+
+	WARN_ON_ONCE(fs_data);
+	ret = __ceph_setxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len, XATTR_CREATE);
+	if (ret == 0)
+		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
+	return ret;
+}
+
+static bool ceph_crypt_empty_dir(struct inode *inode)
+{
+	struct ceph_inode_info *ci = ceph_inode(inode);
+
+	return ci->i_rsubdirs + ci->i_rfiles == 1;
+}
+
+static struct fscrypt_operations ceph_fscrypt_ops = {
+	.get_context		= ceph_crypt_get_context,
+	.set_context		= ceph_crypt_set_context,
+	.empty_dir		= ceph_crypt_empty_dir,
+	.max_namelen		= NAME_MAX,
+};
+
+void ceph_fscrypt_set_ops(struct super_block *sb)
+{
+	fscrypt_set_ops(sb, &ceph_fscrypt_ops);
+}
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
new file mode 100644
index 000000000000..189bd8424284
--- /dev/null
+++ b/fs/ceph/crypto.h
@@ -0,0 +1,24 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Ceph fscrypt functionality
+ */
+
+#ifndef _CEPH_CRYPTO_H
+#define _CEPH_CRYPTO_H
+
+#include <linux/fscrypt.h>
+
+#define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"
+
+#ifdef CONFIG_FS_ENCRYPTION
+void ceph_fscrypt_set_ops(struct super_block *sb);
+
+#else /* CONFIG_FS_ENCRYPTION */
+
+static inline void ceph_fscrypt_set_ops(struct super_block *sb)
+{
+}
+
+#endif /* CONFIG_FS_ENCRYPTION */
+
+#endif
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 5d20a620e96c..d465ad48ade5 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -14,10 +14,12 @@ 
 #include <linux/random.h>
 #include <linux/sort.h>
 #include <linux/iversion.h>
+#include <linux/fscrypt.h>
 
 #include "super.h"
 #include "mds_client.h"
 #include "cache.h"
+#include "crypto.h"
 #include <linux/ceph/decode.h>
 
 /*
@@ -553,6 +555,7 @@  void ceph_evict_inode(struct inode *inode)
 	clear_inode(inode);
 
 	ceph_fscache_unregister_inode_cookie(ci);
+	fscrypt_put_encryption_info(inode);
 
 	__ceph_remove_caps(ci);
 
@@ -912,6 +915,9 @@  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) &&
+		     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.c b/fs/ceph/super.c
index 9b1b7f4cfdd4..cdac6ff675e2 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -20,6 +20,7 @@ 
 #include "super.h"
 #include "mds_client.h"
 #include "cache.h"
+#include "crypto.h"
 
 #include <linux/ceph/ceph_features.h>
 #include <linux/ceph/decode.h>
@@ -988,6 +989,8 @@  static int ceph_set_super(struct super_block *s, struct fs_context *fc)
 	s->s_time_min = 0;
 	s->s_time_max = U32_MAX;
 
+	ceph_fscrypt_set_ops(s);
+
 	ret = set_anon_super_fc(s, fc);
 	if (ret != 0)
 		fsc->sb = NULL;
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 13b02887b085..efe2e963c631 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -1013,6 +1013,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, const 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 24997982de01..d0d719b768e4 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -1359,6 +1359,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, const 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.