ceph-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support
@ 2020-09-04 16:05 Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 01/18] vfs: export new_inode_pseudo Jeff Layton
                   ` (18 more replies)
  0 siblings, 19 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

This is a second posting of the ceph+fscrypt integration work that I've
been experimenting with. The main change with this patch is that I've
based this on top of Eric's fscrypt-pending set. That necessitated a
change to allocate inodes much earlier than we have traditionally, prior
to sending an RPC instead of waiting on the reply.

Note that this just covers the crypto contexts and filenames. I've also
added a patch to encrypt symlink contents as well, but it doesn't seem to
be working correctly.

The file contents work is next, but I'm sort of waiting until some work
to the fscache infrastructure is settled. It would be nice if fscache
also stored encrypted file contents when we plumb this in.

Jeff Layton (18):
  vfs: export new_inode_pseudo
  fscrypt: drop unused inode argument from fscrypt_fname_alloc_buffer
  fscrypt: export fscrypt_d_revalidate
  fscrypt: add fscrypt_new_context_from_inode
  fscrypt: don't balk when inode is already marked encrypted
  fscrypt: move nokey_name conversion to separate function and export it
  lib: lift fscrypt base64 conversion into lib/
  ceph: add fscrypt ioctls
  ceph: crypto context handling for ceph
  ceph: preallocate inode for ops that may create one
  ceph: add routine to create context prior to RPC
  ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  ceph: make ceph_msdc_build_path use ref-walk
  ceph: add encrypted fname handling to ceph_mdsc_build_path
  ceph: make d_revalidate call fscrypt revalidator for encrypted
    dentries
  ceph: add support to readdir for encrypted filenames
  ceph: add fscrypt support to ceph_fill_trace
  ceph: create symlinks with encrypted and base64-encoded targets

 fs/ceph/Makefile             |   1 +
 fs/ceph/crypto.c             | 179 +++++++++++++++++++++++++++++
 fs/ceph/crypto.h             |  83 ++++++++++++++
 fs/ceph/dir.c                | 162 ++++++++++++++++++++------
 fs/ceph/file.c               |  56 +++++----
 fs/ceph/inode.c              | 213 ++++++++++++++++++++++++++++++-----
 fs/ceph/ioctl.c              |  25 ++++
 fs/ceph/mds_client.c         |  75 ++++++++----
 fs/ceph/mds_client.h         |   1 +
 fs/ceph/super.c              |  37 ++++++
 fs/ceph/super.h              |  19 +++-
 fs/ceph/xattr.c              |  32 ++++++
 fs/crypto/Kconfig            |   1 +
 fs/crypto/fname.c            | 139 ++++++++---------------
 fs/crypto/hooks.c            |   2 +-
 fs/crypto/keysetup.c         |   2 +-
 fs/crypto/policy.c           |  20 ++++
 fs/ext4/dir.c                |   2 +-
 fs/ext4/namei.c              |   7 +-
 fs/f2fs/dir.c                |   2 +-
 fs/inode.c                   |   1 +
 fs/ubifs/dir.c               |   2 +-
 include/linux/base64_fname.h |  11 ++
 include/linux/fscrypt.h      |  10 +-
 lib/Kconfig                  |   3 +
 lib/Makefile                 |   1 +
 lib/base64_fname.c           |  71 ++++++++++++
 27 files changed, 943 insertions(+), 214 deletions(-)
 create mode 100644 fs/ceph/crypto.c
 create mode 100644 fs/ceph/crypto.h
 create mode 100644 include/linux/base64_fname.h
 create mode 100644 lib/base64_fname.c

-- 
2.26.2


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  3:38   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 02/18] fscrypt: drop unused inode argument from fscrypt_fname_alloc_buffer Jeff Layton
                   ` (17 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Ceph needs to be able to allocate inodes ahead of a create that might
involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
but it puts the inode on the sb->s_inodes list, and we don't want to
do that until we're ready to insert it into the hash.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/inode.c b/fs/inode.c
index 72c4c347afb7..61554c2477ab 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
 	}
 	return inode;
 }
+EXPORT_SYMBOL(new_inode_pseudo);
 
 /**
  *	new_inode 	- obtain an inode
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 02/18] fscrypt: drop unused inode argument from fscrypt_fname_alloc_buffer
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 01/18] vfs: export new_inode_pseudo Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 03/18] fscrypt: export fscrypt_d_revalidate Jeff Layton
                   ` (16 subsequent siblings)
  18 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/fname.c       | 5 +----
 fs/crypto/hooks.c       | 2 +-
 fs/ext4/dir.c           | 2 +-
 fs/ext4/namei.c         | 7 +++----
 fs/f2fs/dir.c           | 2 +-
 fs/ubifs/dir.c          | 2 +-
 include/linux/fscrypt.h | 5 ++---
 7 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 011830f84d8d..47bcfddb278b 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -260,8 +260,6 @@ bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
 
 /**
  * fscrypt_fname_alloc_buffer() - allocate a buffer for presented filenames
- * @inode: inode of the parent directory (for regular filenames)
- *	   or of the symlink (for symlink targets)
  * @max_encrypted_len: maximum length of encrypted filenames the buffer will be
  *		       used to present
  * @crypto_str: (output) buffer to allocate
@@ -271,8 +269,7 @@ bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
  *
  * Return: 0 on success, -errno on failure
  */
-int fscrypt_fname_alloc_buffer(const struct inode *inode,
-			       u32 max_encrypted_len,
+int fscrypt_fname_alloc_buffer(u32 max_encrypted_len,
 			       struct fscrypt_str *crypto_str)
 {
 	const u32 max_encoded_len = BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX);
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index b69cd29a01a2..c97f914834e6 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -323,7 +323,7 @@ const char *fscrypt_get_symlink(struct inode *inode, const void *caddr,
 	if (cstr.len + sizeof(*sd) - 1 > max_size)
 		return ERR_PTR(-EUCLEAN);
 
-	err = fscrypt_fname_alloc_buffer(inode, cstr.len, &pstr);
+	err = fscrypt_fname_alloc_buffer(cstr.len, &pstr);
 	if (err)
 		return ERR_PTR(err);
 
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 1d82336b1cd4..efe77cffc322 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -148,7 +148,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 	}
 
 	if (IS_ENCRYPTED(inode)) {
-		err = fscrypt_fname_alloc_buffer(inode, EXT4_NAME_LEN, &fstr);
+		err = fscrypt_fname_alloc_buffer(EXT4_NAME_LEN, &fstr);
 		if (err < 0)
 			return err;
 	}
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 153a9fbe1dd0..0d74615fcce3 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -663,8 +663,7 @@ static struct stats dx_show_leaf(struct inode *dir,
 
 					/* Directory is encrypted */
 					res = fscrypt_fname_alloc_buffer(
-						dir, len,
-						&fname_crypto_str);
+						len, &fname_crypto_str);
 					if (res)
 						printk(KERN_WARNING "Error "
 							"allocating crypto "
@@ -1016,8 +1015,8 @@ static int htree_dirblock_to_tree(struct file *dir_file,
 			brelse(bh);
 			return err;
 		}
-		err = fscrypt_fname_alloc_buffer(dir, EXT4_NAME_LEN,
-						     &fname_crypto_str);
+		err = fscrypt_fname_alloc_buffer(EXT4_NAME_LEN,
+						 &fname_crypto_str);
 		if (err < 0) {
 			brelse(bh);
 			return err;
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index d627ca97fc50..414bc94fbd54 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -1032,7 +1032,7 @@ static int f2fs_readdir(struct file *file, struct dir_context *ctx)
 		if (err)
 			goto out;
 
-		err = fscrypt_fname_alloc_buffer(inode, F2FS_NAME_LEN, &fstr);
+		err = fscrypt_fname_alloc_buffer(F2FS_NAME_LEN, &fstr);
 		if (err < 0)
 			goto out;
 	}
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 26739ae3ffee..f2f88ddf1dea 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -509,7 +509,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 		if (err)
 			return err;
 
-		err = fscrypt_fname_alloc_buffer(dir, UBIFS_MAX_NLEN, &fstr);
+		err = fscrypt_fname_alloc_buffer(UBIFS_MAX_NLEN, &fstr);
 		if (err)
 			return err;
 
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 4ee636e9e1fc..81d6ded24328 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -198,7 +198,7 @@ static inline void fscrypt_free_filename(struct fscrypt_name *fname)
 	kfree(fname->crypto_buf.name);
 }
 
-int fscrypt_fname_alloc_buffer(const struct inode *inode, u32 max_encrypted_len,
+int fscrypt_fname_alloc_buffer(u32 max_encrypted_len,
 			       struct fscrypt_str *crypto_str);
 void fscrypt_fname_free_buffer(struct fscrypt_str *crypto_str);
 int fscrypt_fname_disk_to_usr(const struct inode *inode,
@@ -436,8 +436,7 @@ static inline void fscrypt_free_filename(struct fscrypt_name *fname)
 	return;
 }
 
-static inline int fscrypt_fname_alloc_buffer(const struct inode *inode,
-					     u32 max_encrypted_len,
+static inline int fscrypt_fname_alloc_buffer(u32 max_encrypted_len,
 					     struct fscrypt_str *crypto_str)
 {
 	return -EOPNOTSUPP;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 03/18] fscrypt: export fscrypt_d_revalidate
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 01/18] vfs: export new_inode_pseudo Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 02/18] fscrypt: drop unused inode argument from fscrypt_fname_alloc_buffer Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode Jeff Layton
                   ` (15 subsequent siblings)
  18 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

ceph already has its own d_revalidate op so we can't rely on fscrypt
using that directly. Export this symbol so filesystems can call it
from their own d_revalidate op.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/fname.c       | 3 ++-
 include/linux/fscrypt.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 47bcfddb278b..9440a44e24ac 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -538,7 +538,7 @@ EXPORT_SYMBOL_GPL(fscrypt_fname_siphash);
  * Validate dentries in encrypted directories to make sure we aren't potentially
  * caching stale dentries after a key has been added.
  */
-static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
+int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *dir;
 	int err;
@@ -577,6 +577,7 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 
 	return valid;
 }
+EXPORT_SYMBOL_GPL(fscrypt_d_revalidate);
 
 const struct dentry_operations fscrypt_d_ops = {
 	.d_revalidate = fscrypt_d_revalidate,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 81d6ded24328..16d673c50448 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -208,6 +208,7 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 bool fscrypt_match_name(const struct fscrypt_name *fname,
 			const u8 *de_name, u32 de_name_len);
 u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name);
+int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
 
 /* bio.c */
 void fscrypt_decrypt_bio(struct bio *bio);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (2 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 03/18] fscrypt: export fscrypt_d_revalidate Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  3:48   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted Jeff Layton
                   ` (14 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

CephFS will need to be able to generate a context for a new "prepared"
inode. Add a new routine for getting the context out of an in-core
inode.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/policy.c      | 20 ++++++++++++++++++++
 include/linux/fscrypt.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index c56ad886f7d7..10eddd113a21 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
 }
 EXPORT_SYMBOL_GPL(fscrypt_set_context);
 
+/**
+ * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode
+ * @ctx: where context should be written
+ * @inode: inode from which to fetch context
+ *
+ * Given an in-core prepared, but not-necessarily fully-instantiated inode,
+ * generate an encryption context from its policy and write it to ctx.
+ *
+ * Returns size of the context.
+ */
+int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode)
+{
+	struct fscrypt_info *ci = inode->i_crypt_info;
+
+	BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
+
+	return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce);
+}
+EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode);
+
 /**
  * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption'
  * @sb: the filesystem on which test_dummy_encryption is being specified
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 16d673c50448..0ddbd27a2e58 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -157,6 +157,7 @@ int fscrypt_ioctl_get_policy_ex(struct file *filp, void __user *arg);
 int fscrypt_ioctl_get_nonce(struct file *filp, void __user *arg);
 int fscrypt_has_permitted_context(struct inode *parent, struct inode *child);
 int fscrypt_set_context(struct inode *inode, void *fs_data);
+int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode);
 
 struct fscrypt_dummy_context {
 	const union fscrypt_context *ctx;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (3 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  3:52   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it Jeff Layton
                   ` (13 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Cephfs (currently) sets this flag early and only fetches the context
later. Eventually we may not need this, but for now it prevents this
warning from popping.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/keysetup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index ad64525ec680..3b4ec16fc528 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
 		const union fscrypt_context *dummy_ctx =
 			fscrypt_get_dummy_context(inode->i_sb);
 
-		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
+		if (!dummy_ctx) {
 			fscrypt_warn(inode,
 				     "Error %d getting encryption context",
 				     res);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (4 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  3:55   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/ Jeff Layton
                   ` (12 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/fname.c       | 71 +++++++++++++++++++++++------------------
 include/linux/fscrypt.h |  3 ++
 2 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 9440a44e24ac..09f09def87fc 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -300,6 +300,45 @@ void fscrypt_fname_free_buffer(struct fscrypt_str *crypto_str)
 }
 EXPORT_SYMBOL(fscrypt_fname_free_buffer);
 
+void fscrypt_encode_nokey_name(u32 hash, u32 minor_hash,
+			     const struct fscrypt_str *iname,
+			     struct fscrypt_str *oname)
+{
+	struct fscrypt_nokey_name nokey_name;
+	u32 size; /* size of the unencoded no-key name */
+
+	/*
+	 * Sanity check that struct fscrypt_nokey_name doesn't have padding
+	 * between fields and that its encoded size never exceeds NAME_MAX.
+	 */
+	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, dirhash) !=
+		     offsetof(struct fscrypt_nokey_name, bytes));
+	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, bytes) !=
+		     offsetof(struct fscrypt_nokey_name, sha256));
+	BUILD_BUG_ON(BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX) > NAME_MAX);
+
+	if (hash) {
+		nokey_name.dirhash[0] = hash;
+		nokey_name.dirhash[1] = minor_hash;
+	} else {
+		nokey_name.dirhash[0] = 0;
+		nokey_name.dirhash[1] = 0;
+	}
+	if (iname->len <= sizeof(nokey_name.bytes)) {
+		memcpy(nokey_name.bytes, iname->name, iname->len);
+		size = offsetof(struct fscrypt_nokey_name, bytes[iname->len]);
+	} else {
+		memcpy(nokey_name.bytes, iname->name, sizeof(nokey_name.bytes));
+		/* Compute strong hash of remaining part of name. */
+		fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)],
+				  iname->len - sizeof(nokey_name.bytes),
+				  nokey_name.sha256);
+		size = FSCRYPT_NOKEY_NAME_MAX;
+	}
+	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
+}
+EXPORT_SYMBOL(fscrypt_encode_nokey_name);
+
 /**
  * fscrypt_fname_disk_to_usr() - convert an encrypted filename to
  *				 user-presentable form
@@ -327,8 +366,6 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 			      struct fscrypt_str *oname)
 {
 	const struct qstr qname = FSTR_TO_QSTR(iname);
-	struct fscrypt_nokey_name nokey_name;
-	u32 size; /* size of the unencoded no-key name */
 
 	if (fscrypt_is_dot_dotdot(&qname)) {
 		oname->name[0] = '.';
@@ -343,35 +380,7 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 	if (fscrypt_has_encryption_key(inode))
 		return fname_decrypt(inode, iname, oname);
 
-	/*
-	 * Sanity check that struct fscrypt_nokey_name doesn't have padding
-	 * between fields and that its encoded size never exceeds NAME_MAX.
-	 */
-	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, dirhash) !=
-		     offsetof(struct fscrypt_nokey_name, bytes));
-	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, bytes) !=
-		     offsetof(struct fscrypt_nokey_name, sha256));
-	BUILD_BUG_ON(BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX) > NAME_MAX);
-
-	if (hash) {
-		nokey_name.dirhash[0] = hash;
-		nokey_name.dirhash[1] = minor_hash;
-	} else {
-		nokey_name.dirhash[0] = 0;
-		nokey_name.dirhash[1] = 0;
-	}
-	if (iname->len <= sizeof(nokey_name.bytes)) {
-		memcpy(nokey_name.bytes, iname->name, iname->len);
-		size = offsetof(struct fscrypt_nokey_name, bytes[iname->len]);
-	} else {
-		memcpy(nokey_name.bytes, iname->name, sizeof(nokey_name.bytes));
-		/* Compute strong hash of remaining part of name. */
-		fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)],
-				  iname->len - sizeof(nokey_name.bytes),
-				  nokey_name.sha256);
-		size = FSCRYPT_NOKEY_NAME_MAX;
-	}
-	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
+	fscrypt_encode_nokey_name(hash, minor_hash, iname, oname);
 	return 0;
 }
 EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 0ddbd27a2e58..57146f9f70e7 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -202,6 +202,9 @@ static inline void fscrypt_free_filename(struct fscrypt_name *fname)
 int fscrypt_fname_alloc_buffer(u32 max_encrypted_len,
 			       struct fscrypt_str *crypto_str);
 void fscrypt_fname_free_buffer(struct fscrypt_str *crypto_str);
+void fscrypt_encode_nokey_name(u32 hash, u32 minor_hash,
+			     const struct fscrypt_str *iname,
+			     struct fscrypt_str *oname);
 int fscrypt_fname_disk_to_usr(const struct inode *inode,
 			      u32 hash, u32 minor_hash,
 			      const struct fscrypt_str *iname,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (5 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  3:59   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 08/18] ceph: add fscrypt ioctls Jeff Layton
                   ` (11 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Once we allow encrypted filenames on ceph we'll end up with names that
may have illegal characters in them (embedded '\0' or '/'), or
characters that aren't printable.

It will be safer to use strings that are printable. It turns out that the
MDS doesn't really care about the length of filenames, so we can just
base64 encode and decode filenames before writing and reading them.

Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
select it when it's enabled.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/Kconfig            |  1 +
 fs/crypto/fname.c            | 64 ++------------------------------
 include/linux/base64_fname.h | 11 ++++++
 lib/Kconfig                  |  3 ++
 lib/Makefile                 |  1 +
 lib/base64_fname.c           | 71 ++++++++++++++++++++++++++++++++++++
 6 files changed, 90 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/base64_fname.h
 create mode 100644 lib/base64_fname.c

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index a5f5c30368a2..6b27d105420c 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -6,6 +6,7 @@ config FS_ENCRYPTION
 	select CRYPTO_SKCIPHER
 	select CRYPTO_LIB_SHA256
 	select KEYS
+	select BASE64_FNAME
 	help
 	  Enable encryption of files and directories.  This
 	  feature is similar to ecryptfs, but it is more memory
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 09f09def87fc..89e26e923547 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -11,6 +11,7 @@
  * This has not yet undergone a rigorous security audit.
  */
 
+#include <linux/base64_fname.h>
 #include <linux/namei.h>
 #include <linux/scatterlist.h>
 #include <crypto/hash.h>
@@ -184,64 +185,6 @@ static int fname_decrypt(const struct inode *inode,
 	return 0;
 }
 
-static const char lookup_table[65] =
-	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
-
-#define BASE64_CHARS(nbytes)	DIV_ROUND_UP((nbytes) * 4, 3)
-
-/**
- * base64_encode() - base64-encode some bytes
- * @src: the bytes to encode
- * @len: number of bytes to encode
- * @dst: (output) the base64-encoded string.  Not NUL-terminated.
- *
- * Encodes the input string using characters from the set [A-Za-z0-9+,].
- * The encoded string is roughly 4/3 times the size of the input string.
- *
- * Return: length of the encoded string
- */
-static int base64_encode(const u8 *src, int len, char *dst)
-{
-	int i, bits = 0, ac = 0;
-	char *cp = dst;
-
-	for (i = 0; i < len; i++) {
-		ac += src[i] << bits;
-		bits += 8;
-		do {
-			*cp++ = lookup_table[ac & 0x3f];
-			ac >>= 6;
-			bits -= 6;
-		} while (bits >= 6);
-	}
-	if (bits)
-		*cp++ = lookup_table[ac & 0x3f];
-	return cp - dst;
-}
-
-static int base64_decode(const char *src, int len, u8 *dst)
-{
-	int i, bits = 0, ac = 0;
-	const char *p;
-	u8 *cp = dst;
-
-	for (i = 0; i < len; i++) {
-		p = strchr(lookup_table, src[i]);
-		if (p == NULL || src[i] == 0)
-			return -2;
-		ac += (p - lookup_table) << bits;
-		bits += 6;
-		if (bits >= 8) {
-			*cp++ = ac & 0xff;
-			ac >>= 8;
-			bits -= 8;
-		}
-	}
-	if (ac)
-		return -1;
-	return cp - dst;
-}
-
 bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
 				  u32 max_len, u32 *encrypted_len_ret)
 {
@@ -335,7 +278,7 @@ void fscrypt_encode_nokey_name(u32 hash, u32 minor_hash,
 				  nokey_name.sha256);
 		size = FSCRYPT_NOKEY_NAME_MAX;
 	}
-	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
+	oname->len = base64_encode_fname((const u8 *)&nokey_name, size, oname->name);
 }
 EXPORT_SYMBOL(fscrypt_encode_nokey_name);
 
@@ -380,7 +323,6 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 	if (fscrypt_has_encryption_key(inode))
 		return fname_decrypt(inode, iname, oname);
 
-	fscrypt_encode_nokey_name(hash, minor_hash, iname, oname);
 	return 0;
 }
 EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
@@ -460,7 +402,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	if (fname->crypto_buf.name == NULL)
 		return -ENOMEM;
 
-	ret = base64_decode(iname->name, iname->len, fname->crypto_buf.name);
+	ret = base64_decode_fname(iname->name, iname->len, fname->crypto_buf.name);
 	if (ret < (int)offsetof(struct fscrypt_nokey_name, bytes[1]) ||
 	    (ret > offsetof(struct fscrypt_nokey_name, sha256) &&
 	     ret != FSCRYPT_NOKEY_NAME_MAX)) {
diff --git a/include/linux/base64_fname.h b/include/linux/base64_fname.h
new file mode 100644
index 000000000000..d98d79b4c95c
--- /dev/null
+++ b/include/linux/base64_fname.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#ifndef _LIB_BASE64_FNAME_H
+#define _LIB_BASE64_FNAME_H
+#include <linux/types.h>
+
+#define BASE64_CHARS(nbytes)	DIV_ROUND_UP((nbytes) * 4, 3)
+
+int base64_encode_fname(const u8 *src, int len, char *dst);
+int base64_decode_fname(const char *src, int len, u8 *dst);
+#endif /* _LIB_BASE64_FNAME_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b4b98a03ff98..94f3939c4cfa 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -684,3 +684,6 @@ config GENERIC_LIB_UCMPDI2
 config PLDMFW
 	bool
 	default n
+
+config BASE64_FNAME
+	tristate
diff --git a/lib/Makefile b/lib/Makefile
index a4a4c6864f51..4e77167d6252 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -171,6 +171,7 @@ obj-$(CONFIG_LIBCRC32C)	+= libcrc32c.o
 obj-$(CONFIG_CRC8)	+= crc8.o
 obj-$(CONFIG_XXHASH)	+= xxhash.o
 obj-$(CONFIG_GENERIC_ALLOCATOR) += genalloc.o
+obj-$(CONFIG_BASE64_FNAME) += base64_fname.o
 
 obj-$(CONFIG_842_COMPRESS) += 842/
 obj-$(CONFIG_842_DECOMPRESS) += 842/
diff --git a/lib/base64_fname.c b/lib/base64_fname.c
new file mode 100644
index 000000000000..7638c45e4035
--- /dev/null
+++ b/lib/base64_fname.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Modified base64 encode/decode functions, suitable for use as filename components.
+ *
+ * Originally lifted from fs/crypto/fname.c
+ *
+ * Copyright (C) 2015, Jaegeuk Kim
+ * Copyright (C) 2015, Eric Biggers
+ */
+
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/string.h>
+
+static const char lookup_table[65] =
+	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
+
+/**
+ * base64_encode() - base64-encode some bytes
+ * @src: the bytes to encode
+ * @len: number of bytes to encode
+ * @dst: (output) the base64-encoded string.  Not NUL-terminated.
+ *
+ * Encodes the input string using characters from the set [A-Za-z0-9+,].
+ * The encoded string is roughly 4/3 times the size of the input string.
+ *
+ * Return: length of the encoded string
+ */
+int base64_encode_fname(const u8 *src, int len, char *dst)
+{
+	int i, bits = 0, ac = 0;
+	char *cp = dst;
+
+	for (i = 0; i < len; i++) {
+		ac += src[i] << bits;
+		bits += 8;
+		do {
+			*cp++ = lookup_table[ac & 0x3f];
+			ac >>= 6;
+			bits -= 6;
+		} while (bits >= 6);
+	}
+	if (bits)
+		*cp++ = lookup_table[ac & 0x3f];
+	return cp - dst;
+}
+EXPORT_SYMBOL(base64_encode_fname);
+
+int base64_decode_fname(const char *src, int len, u8 *dst)
+{
+	int i, bits = 0, ac = 0;
+	const char *p;
+	u8 *cp = dst;
+
+	for (i = 0; i < len; i++) {
+		p = strchr(lookup_table, src[i]);
+		if (p == NULL || src[i] == 0)
+			return -2;
+		ac += (p - lookup_table) << bits;
+		bits += 6;
+		if (bits >= 8) {
+			*cp++ = ac & 0xff;
+			ac >>= 8;
+			bits -= 8;
+		}
+	}
+	if (ac)
+		return -1;
+	return cp - dst;
+}
+EXPORT_SYMBOL(base64_decode_fname);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 08/18] ceph: add fscrypt ioctls
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (6 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/ Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 09/18] ceph: crypto context handling for ceph Jeff Layton
                   ` (10 subsequent siblings)
  18 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Boilerplate ioctls for controlling encryption.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/ioctl.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c
index 6e061bf62ad4..381e44b2d60a 100644
--- a/fs/ceph/ioctl.c
+++ b/fs/ceph/ioctl.c
@@ -6,6 +6,7 @@
 #include "mds_client.h"
 #include "ioctl.h"
 #include <linux/ceph/striper.h>
+#include <linux/fscrypt.h>
 
 /*
  * ioctls
@@ -289,6 +290,30 @@ long ceph_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	case CEPH_IOC_SYNCIO:
 		return ceph_ioctl_syncio(file);
+
+	case FS_IOC_SET_ENCRYPTION_POLICY:
+		return fscrypt_ioctl_set_policy(file, (const void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_POLICY:
+		return fscrypt_ioctl_get_policy(file, (void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_POLICY_EX:
+		return fscrypt_ioctl_get_policy_ex(file, (void __user *)arg);
+
+	case FS_IOC_ADD_ENCRYPTION_KEY:
+		return fscrypt_ioctl_add_key(file, (void __user *)arg);
+
+	case FS_IOC_REMOVE_ENCRYPTION_KEY:
+		return fscrypt_ioctl_remove_key(file, (void __user *)arg);
+
+	case FS_IOC_REMOVE_ENCRYPTION_KEY_ALL_USERS:
+		return fscrypt_ioctl_remove_key_all_users(file, (void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_KEY_STATUS:
+		return fscrypt_ioctl_get_key_status(file, (void __user *)arg);
+
+	case FS_IOC_GET_ENCRYPTION_NONCE:
+		return fscrypt_ioctl_get_nonce(file, (void __user *)arg);
 	}
 
 	return -ENOTTY;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 09/18] ceph: crypto context handling for ceph
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (7 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 08/18] ceph: add fscrypt ioctls Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  4:29   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 10/18] ceph: preallocate inode for ops that may create one Jeff Layton
                   ` (9 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

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

Also add support for "dummy" encryption (useful for testing with
automated test harnesses like xfstests).

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/Makefile |  1 +
 fs/ceph/crypto.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ceph/crypto.h | 24 +++++++++++++++++
 fs/ceph/inode.c  |  1 +
 fs/ceph/super.c  | 37 ++++++++++++++++++++++++++
 fs/ceph/super.h  |  7 ++++-
 6 files changed, 138 insertions(+), 1 deletion(-)
 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..22a09d422b72
--- /dev/null
+++ b/fs/ceph/crypto.c
@@ -0,0 +1,69 @@
+// 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)
+{
+	int ret = __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);
+
+	if (ret > 0)
+		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
+	return ret;
+}
+
+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 const union fscrypt_context *
+ceph_get_dummy_context(struct super_block *sb)
+{
+	return ceph_sb_to_client(sb)->dummy_enc_ctx.ctx;
+}
+
+static struct fscrypt_operations ceph_fscrypt_ops = {
+	.key_prefix		= "ceph:",
+	.get_context		= ceph_crypt_get_context,
+	.set_context		= ceph_crypt_set_context,
+	.get_dummy_context	= ceph_get_dummy_context,
+	.empty_dir		= ceph_crypt_empty_dir,
+	.max_namelen		= NAME_MAX,
+};
+
+int ceph_fscrypt_set_ops(struct super_block *sb)
+{
+	struct ceph_fs_client *fsc = sb->s_fs_info;
+
+	fscrypt_set_ops(sb, &ceph_fscrypt_ops);
+
+	if (ceph_test_mount_opt(fsc, TEST_DUMMY_ENC)) {
+		substring_t arg = { };
+
+		/* Ewwwwwwww */
+		if (fsc->mount_options->test_dummy_encryption) {
+			arg.from = fsc->mount_options->test_dummy_encryption;
+			arg.to = arg.from + strlen(arg.from) - 1;
+		}
+
+		return fscrypt_set_test_dummy_encryption(sb, &arg, &fsc->dummy_enc_ctx);
+	}
+	return 0;
+}
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
new file mode 100644
index 000000000000..af06dca5f5a6
--- /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
+
+#ifdef CONFIG_FS_ENCRYPTION
+
+#define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"
+
+int ceph_fscrypt_set_ops(struct super_block *sb);
+
+#else /* CONFIG_FS_ENCRYPTION */
+
+static inline int ceph_fscrypt_set_ops(struct super_block *sb)
+{
+	return 0;
+}
+
+#endif /* CONFIG_FS_ENCRYPTION */
+
+#endif
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 156b98bda6aa..a527c5dbf93f 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -543,6 +543,7 @@ void ceph_evict_inode(struct inode *inode)
 
 	dout("evict_inode %p ino %llx.%llx\n", inode, ceph_vinop(inode));
 
+	fscrypt_put_encryption_info(inode);
 	truncate_inode_pages_final(&inode->i_data);
 	clear_inode(inode);
 
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 7ec0e6d03d10..95f5a7cf60f2 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>
@@ -44,6 +45,7 @@ static void ceph_put_super(struct super_block *s)
 	struct ceph_fs_client *fsc = ceph_sb_to_client(s);
 
 	dout("put_super\n");
+	fscrypt_free_dummy_context(&fsc->dummy_enc_ctx);
 	ceph_mdsc_close_sessions(fsc->mdsc);
 }
 
@@ -159,6 +161,7 @@ enum {
 	Opt_quotadf,
 	Opt_copyfrom,
 	Opt_wsync,
+	Opt_test_dummy_encryption,
 };
 
 enum ceph_recover_session_mode {
@@ -197,6 +200,8 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
 	fsparam_u32	("rsize",			Opt_rsize),
 	fsparam_string	("snapdirname",			Opt_snapdirname),
 	fsparam_string	("source",			Opt_source),
+	fsparam_flag_no ("test_dummy_encryption",	Opt_test_dummy_encryption),
+	fsparam_string	("test_dummy_encryption",	Opt_test_dummy_encryption),
 	fsparam_u32	("wsize",			Opt_wsize),
 	fsparam_flag_no	("wsync",			Opt_wsync),
 	{}
@@ -455,6 +460,21 @@ static int ceph_parse_mount_param(struct fs_context *fc,
 		else
 			fsopt->flags |= CEPH_MOUNT_OPT_ASYNC_DIROPS;
 		break;
+	case Opt_test_dummy_encryption:
+		kfree(fsopt->test_dummy_encryption);
+		fsopt->test_dummy_encryption = NULL;
+		if (!result.negated) {
+#ifdef CONFIG_FS_ENCRYPTION
+			fsopt->test_dummy_encryption = param->string;
+			param->string = NULL;
+			fsopt->flags |= CEPH_MOUNT_OPT_TEST_DUMMY_ENC;
+#else
+			return warnfc(fc, "FS encryption not supported: test_dummy_encryption mount option ignored");
+#endif
+		} else {
+			fsopt->flags &= ~CEPH_MOUNT_OPT_TEST_DUMMY_ENC;
+		}
+		break;
 	default:
 		BUG();
 	}
@@ -474,6 +494,7 @@ static void destroy_mount_options(struct ceph_mount_options *args)
 	kfree(args->mds_namespace);
 	kfree(args->server_path);
 	kfree(args->fscache_uniq);
+	kfree(args->test_dummy_encryption);
 	kfree(args);
 }
 
@@ -581,6 +602,8 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
 	if (fsopt->flags & CEPH_MOUNT_OPT_ASYNC_DIROPS)
 		seq_puts(m, ",nowsync");
 
+	fscrypt_show_test_dummy_encryption(m, ',', root->d_sb);
+
 	if (fsopt->wsize != CEPH_MAX_WRITE_SIZE)
 		seq_printf(m, ",wsize=%u", fsopt->wsize);
 	if (fsopt->rsize != CEPH_MAX_READ_SIZE)
@@ -984,7 +1007,12 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
 	s->s_time_min = 0;
 	s->s_time_max = U32_MAX;
 
+	ret = ceph_fscrypt_set_ops(s);
+	if (ret)
+		goto out;
+
 	ret = set_anon_super_fc(s, fc);
+out:
 	if (ret != 0)
 		fsc->sb = NULL;
 	return ret;
@@ -1140,6 +1168,15 @@ static int ceph_reconfigure_fc(struct fs_context *fc)
 	else
 		ceph_clear_mount_opt(fsc, ASYNC_DIROPS);
 
+	/* Don't allow test_dummy_encryption to change on remount */
+	if (fsopt->flags & CEPH_MOUNT_OPT_TEST_DUMMY_ENC) {
+		if (!ceph_test_mount_opt(fsc, TEST_DUMMY_ENC))
+			return -EEXIST;
+	} else {
+		if (ceph_test_mount_opt(fsc, TEST_DUMMY_ENC))
+			return -EEXIST;
+	}
+
 	sync_filesystem(fc->root->d_sb);
 	return 0;
 }
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index b3aa2395b66e..3b8ffa6aee46 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -17,6 +17,7 @@
 #include <linux/posix_acl.h>
 #include <linux/refcount.h>
 #include <linux/security.h>
+#include <linux/fscrypt.h>
 
 #include <linux/ceph/libceph.h>
 
@@ -44,6 +45,7 @@
 #define CEPH_MOUNT_OPT_NOQUOTADF       (1<<13) /* no root dir quota in statfs */
 #define CEPH_MOUNT_OPT_NOCOPYFROM      (1<<14) /* don't use RADOS 'copy-from' op */
 #define CEPH_MOUNT_OPT_ASYNC_DIROPS    (1<<15) /* allow async directory ops */
+#define CEPH_MOUNT_OPT_TEST_DUMMY_ENC  (1<<16) /* enable dummy encryption (for testing) */
 
 #define CEPH_MOUNT_OPT_DEFAULT			\
 	(CEPH_MOUNT_OPT_DCACHE |		\
@@ -96,6 +98,7 @@ struct ceph_mount_options {
 	char *mds_namespace;  /* default NULL */
 	char *server_path;    /* default NULL (means "/") */
 	char *fscache_uniq;   /* default NULL */
+	char *test_dummy_encryption;	/* default NULL */
 };
 
 struct ceph_fs_client {
@@ -135,9 +138,11 @@ struct ceph_fs_client {
 #ifdef CONFIG_CEPH_FSCACHE
 	struct fscache_cookie *fscache;
 #endif
+#ifdef CONFIG_FS_ENCRYPTION
+	struct fscrypt_dummy_context dummy_enc_ctx;
+#endif
 };
 
-
 /*
  * File i/o capability.  This tracks shared state with the metadata
  * server that allows us to cache or writeback attributes or to read
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 10/18] ceph: preallocate inode for ops that may create one
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (8 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 09/18] ceph: crypto context handling for ceph Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC Jeff Layton
                   ` (8 subsequent siblings)
  18 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

When creating a new inode, we need to determine the crypto context
before we can transmit the RPC. The fscrypt API has a routine for getting
a crypto context before a create occurs, but it requires an inode.

Change the ceph code to preallocate an inode in advance of a create of
any sort (open(), mknod(), symlink(), etc). Move the existing code that
generates the ACL and SELinux blobs into this routine since that's
mostly common across all the different codepaths.

In most cases, we just want to allow ceph_fill_trace to use that inode
after the reply comes in, so add a new field to the MDS request for it
(r_new_inode).

The async create codepath is a bit different though. In that case, we
want to hash the inode in advance of the RPC so that it can be used
before the reply comes in. If the call subsequently fails with
-EJUKEBOX, then just put the references and clean up the as_ctx. Note
that with this change, we now need to regenerate the as_ctx when this
occurs, but it's quite rare for it to happen.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c        | 49 ++++++++++++++++++--------------
 fs/ceph/file.c       | 56 +++++++++++++++++++++++--------------
 fs/ceph/inode.c      | 66 +++++++++++++++++++++++++++++++++++++++-----
 fs/ceph/mds_client.c |  1 +
 fs/ceph/mds_client.h |  1 +
 fs/ceph/super.h      |  5 +++-
 6 files changed, 130 insertions(+), 48 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 040eaad9d063..b3f2741becdb 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -841,13 +841,6 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
 		goto out;
 	}
 
-	err = ceph_pre_init_acls(dir, &mode, &as_ctx);
-	if (err < 0)
-		goto out;
-	err = ceph_security_init_secctx(dentry, mode, &as_ctx);
-	if (err < 0)
-		goto out;
-
 	dout("mknod in dir %p dentry %p mode 0%ho rdev %d\n",
 	     dir, dentry, mode, rdev);
 	req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_MKNOD, USE_AUTH_MDS);
@@ -855,6 +848,14 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
 		err = PTR_ERR(req);
 		goto out;
 	}
+
+	req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx);
+	if (IS_ERR(req->r_new_inode)) {
+		err = PTR_ERR(req->r_new_inode);
+		req->r_new_inode = NULL;
+		goto out_req;
+	}
+
 	req->r_dentry = dget(dentry);
 	req->r_num_caps = 2;
 	req->r_parent = dir;
@@ -870,6 +871,7 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
 	err = ceph_mdsc_do_request(mdsc, dir, req);
 	if (!err && !req->r_reply_info.head->is_dentry)
 		err = ceph_handle_notrace_create(dir, dentry);
+out_req:
 	ceph_mdsc_put_request(req);
 out:
 	if (!err)
@@ -893,6 +895,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 	struct ceph_mds_client *mdsc = fsc->mdsc;
 	struct ceph_mds_request *req;
 	struct ceph_acl_sec_ctx as_ctx = {};
+	umode_t mode = S_IFLNK | 0777;
 	int err;
 
 	if (ceph_snap(dir) != CEPH_NOSNAP)
@@ -903,21 +906,24 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 		goto out;
 	}
 
-	err = ceph_security_init_secctx(dentry, S_IFLNK | 0777, &as_ctx);
-	if (err < 0)
-		goto out;
-
 	dout("symlink in dir %p dentry %p to '%s'\n", dir, dentry, dest);
 	req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SYMLINK, USE_AUTH_MDS);
 	if (IS_ERR(req)) {
 		err = PTR_ERR(req);
 		goto out;
 	}
+
+	req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx);
+	if (IS_ERR(req->r_new_inode)) {
+		err = PTR_ERR(req->r_new_inode);
+		req->r_new_inode = NULL;
+		goto out_req;
+	}
+
 	req->r_path2 = kstrdup(dest, GFP_KERNEL);
 	if (!req->r_path2) {
 		err = -ENOMEM;
-		ceph_mdsc_put_request(req);
-		goto out;
+		goto out_req;
 	}
 	req->r_parent = dir;
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
@@ -932,6 +938,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 	err = ceph_mdsc_do_request(mdsc, dir, req);
 	if (!err && !req->r_reply_info.head->is_dentry)
 		err = ceph_handle_notrace_create(dir, dentry);
+out_req:
 	ceph_mdsc_put_request(req);
 out:
 	if (err)
@@ -967,13 +974,6 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 		goto out;
 	}
 
-	mode |= S_IFDIR;
-	err = ceph_pre_init_acls(dir, &mode, &as_ctx);
-	if (err < 0)
-		goto out;
-	err = ceph_security_init_secctx(dentry, mode, &as_ctx);
-	if (err < 0)
-		goto out;
 
 	req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
 	if (IS_ERR(req)) {
@@ -981,6 +981,14 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 		goto out;
 	}
 
+	mode |= S_IFDIR;
+	req->r_new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx);
+	if (IS_ERR(req->r_new_inode)) {
+		err = PTR_ERR(req->r_new_inode);
+		req->r_new_inode = NULL;
+		goto out_req;
+	}
+
 	req->r_dentry = dget(dentry);
 	req->r_num_caps = 2;
 	req->r_parent = dir;
@@ -997,6 +1005,7 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 	    !req->r_reply_info.head->is_target &&
 	    !req->r_reply_info.head->is_dentry)
 		err = ceph_handle_notrace_create(dir, dentry);
+out_req:
 	ceph_mdsc_put_request(req);
 out:
 	if (!err)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 69dc9516c1f5..e15fa0df92ca 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -566,7 +566,8 @@ static void ceph_async_create_cb(struct ceph_mds_client *mdsc,
 	ceph_mdsc_release_dir_caps(req);
 }
 
-static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
+static int ceph_finish_async_create(struct inode *dir, struct inode *inode,
+				    struct dentry *dentry,
 				    struct file *file, umode_t mode,
 				    struct ceph_mds_request *req,
 				    struct ceph_acl_sec_ctx *as_ctx,
@@ -577,17 +578,12 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 	struct ceph_mds_reply_inode in = { };
 	struct ceph_mds_reply_info_in iinfo = { .in = &in };
 	struct ceph_inode_info *ci = ceph_inode(dir);
-	struct inode *inode;
 	struct timespec64 now;
 	struct ceph_vino vino = { .ino = req->r_deleg_ino,
 				  .snap = CEPH_NOSNAP };
 
 	ktime_get_real_ts64(&now);
 
-	inode = ceph_get_inode(dentry->d_sb, vino);
-	if (IS_ERR(inode))
-		return PTR_ERR(inode);
-
 	iinfo.inline_version = CEPH_INLINE_NONE;
 	iinfo.change_attr = 1;
 	ceph_encode_timespec64(&iinfo.btime, &now);
@@ -623,8 +619,7 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 		ceph_dir_clear_complete(dir);
 		if (!d_unhashed(dentry))
 			d_drop(dentry);
-		if (inode->i_state & I_NEW)
-			discard_new_inode(inode);
+		discard_new_inode(inode);
 	} else {
 		struct dentry *dn;
 
@@ -664,6 +659,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 	struct ceph_fs_client *fsc = ceph_sb_to_client(dir->i_sb);
 	struct ceph_mds_client *mdsc = fsc->mdsc;
 	struct ceph_mds_request *req;
+	struct inode *new_inode = NULL;
 	struct dentry *dn;
 	struct ceph_acl_sec_ctx as_ctx = {};
 	bool try_async = ceph_test_mount_opt(fsc, ASYNC_DIROPS);
@@ -676,21 +672,21 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 
 	if (dentry->d_name.len > NAME_MAX)
 		return -ENAMETOOLONG;
-
+retry:
 	if (flags & O_CREAT) {
 		if (ceph_quota_is_max_files_exceeded(dir))
 			return -EDQUOT;
-		err = ceph_pre_init_acls(dir, &mode, &as_ctx);
-		if (err < 0)
-			return err;
-		err = ceph_security_init_secctx(dentry, mode, &as_ctx);
-		if (err < 0)
+
+		new_inode = ceph_new_inode(dir, dentry, &mode, &as_ctx);
+		if (IS_ERR(new_inode)) {
+			err = PTR_ERR(new_inode);
 			goto out_ctx;
+		}
 	} else if (!d_in_lookup(dentry)) {
 		/* If it's not being looked up, it's negative */
 		return -ENOENT;
 	}
-retry:
+
 	/* do the open */
 	req = prepare_open_request(dir->i_sb, flags, mode);
 	if (IS_ERR(req)) {
@@ -714,21 +710,38 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 			req->r_pagelist = as_ctx.pagelist;
 			as_ctx.pagelist = NULL;
 		}
-		if (try_async &&
-		    (req->r_dir_caps =
-		      try_prep_async_create(dir, dentry, &lo,
-					    &req->r_deleg_ino))) {
+
+		if (try_async && (req->r_dir_caps =
+				  try_prep_async_create(dir, dentry, &lo, &req->r_deleg_ino))) {
+			struct ceph_vino vino = { .ino = req->r_deleg_ino,
+						  .snap = CEPH_NOSNAP };
+
 			set_bit(CEPH_MDS_R_ASYNC, &req->r_req_flags);
 			req->r_args.open.flags |= cpu_to_le32(CEPH_O_EXCL);
 			req->r_callback = ceph_async_create_cb;
+
+			/* Hash inode before RPC */
+			new_inode = ceph_get_inode(dir->i_sb, vino, new_inode);
+			if (IS_ERR(new_inode)) {
+				err = PTR_ERR(new_inode);
+				new_inode = NULL;
+				goto out_req;
+			}
+			WARN_ON_ONCE(!(new_inode->i_state & I_NEW));
+
 			err = ceph_mdsc_submit_request(mdsc, dir, req);
 			if (!err) {
-				err = ceph_finish_async_create(dir, dentry,
+				err = ceph_finish_async_create(dir, new_inode, dentry,
 							file, mode, req,
 							&as_ctx, &lo);
+				new_inode = NULL;
 			} else if (err == -EJUKEBOX) {
 				restore_deleg_ino(dir, req->r_deleg_ino);
 				ceph_mdsc_put_request(req);
+				discard_new_inode(new_inode);
+				ceph_release_acl_sec_ctx(&as_ctx);
+				memset(&as_ctx, 0, sizeof(as_ctx));
+				new_inode = NULL;
 				try_async = false;
 				goto retry;
 			}
@@ -737,6 +750,8 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 	}
 
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
+	req->r_new_inode = new_inode;
+	new_inode = NULL;
 	err = ceph_mdsc_do_request(mdsc,
 				   (flags & (O_CREAT|O_TRUNC)) ? dir : NULL,
 				   req);
@@ -774,6 +789,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 	}
 out_req:
 	ceph_mdsc_put_request(req);
+	iput(new_inode);
 out_ctx:
 	ceph_release_acl_sec_ctx(&as_ctx);
 	dout("atomic_open result=%d\n", err);
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index a527c5dbf93f..e3c81b950f74 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -49,15 +49,67 @@ static int ceph_set_ino_cb(struct inode *inode, void *data)
 	return 0;
 }
 
-struct inode *ceph_get_inode(struct super_block *sb, struct ceph_vino vino)
+/**
+ * ceph_new_inode - allocate a new inode in advance of an expected create
+ * @dir: parent directory for new inode
+ * @mode: mode of new inode
+ */
+struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
+			     umode_t *mode, struct ceph_acl_sec_ctx *as_ctx)
 {
+	int err;
 	struct inode *inode;
 
-	inode = iget5_locked(sb, (unsigned long)vino.ino, ceph_ino_compare,
-			     ceph_set_ino_cb, &vino);
+	inode = new_inode_pseudo(dir->i_sb);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
+	if (!S_ISLNK(*mode)) {
+		err = ceph_pre_init_acls(dir, mode, as_ctx);
+		if (err < 0)
+			goto out_err;
+	}
+
+	err = ceph_security_init_secctx(dentry, *mode, as_ctx);
+	if (err < 0)
+		goto out_err;
+
+	inode->i_state = 0;
+	inode->i_mode = *mode;
+	return inode;
+out_err:
+	iput(inode);
+	return ERR_PTR(err);
+}
+
+/**
+ * ceph_get_inode - find or create/hash a new inode
+ * @sb: superblock to search and allocate in
+ * @vino: vino to search for
+ * @new: optional new inode to insert if one isn't found (may be NULL)
+ *
+ * Search for or insert a new inode into the hash for the given vino, and return a
+ * reference to it. If new is non-NULL, its reference is consumed.
+ */
+struct inode *ceph_get_inode(struct super_block *sb, struct ceph_vino vino, struct inode *new)
+{
+	struct inode *inode;
+
+	if (new) {
+		inode = inode_insert5(new, (unsigned long)vino.ino, ceph_ino_compare,
+					ceph_set_ino_cb, &vino);
+		if (inode != new)
+			iput(new);
+	} else {
+		inode = iget5_locked(sb, (unsigned long)vino.ino, ceph_ino_compare,
+				     ceph_set_ino_cb, &vino);
+	}
+
+	if (!inode) {
+		dout("No inode found for %llx.%llx\n", vino.ino, vino.snap);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	dout("get_inode on %llu=%llx.%llx got %p new %d\n", ceph_present_inode(inode),
 	     ceph_vinop(inode), inode, !!(inode->i_state & I_NEW));
 	return inode;
@@ -72,7 +124,7 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 		.ino = ceph_ino(parent),
 		.snap = CEPH_SNAPDIR,
 	};
-	struct inode *inode = ceph_get_inode(parent->i_sb, vino);
+	struct inode *inode = ceph_get_inode(parent->i_sb, vino, NULL);
 	struct ceph_inode_info *ci = ceph_inode(inode);
 
 	BUG_ON(!S_ISDIR(parent->i_mode));
@@ -1313,7 +1365,7 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 		tvino.ino = le64_to_cpu(rinfo->targeti.in->ino);
 		tvino.snap = le64_to_cpu(rinfo->targeti.in->snapid);
 
-		in = ceph_get_inode(sb, tvino);
+		in = ceph_get_inode(sb, tvino, xchg(&req->r_new_inode, NULL));
 		if (IS_ERR(in)) {
 			err = PTR_ERR(in);
 			goto done;
@@ -1507,7 +1559,7 @@ static int readdir_prepopulate_inodes_only(struct ceph_mds_request *req,
 		vino.ino = le64_to_cpu(rde->inode.in->ino);
 		vino.snap = le64_to_cpu(rde->inode.in->snapid);
 
-		in = ceph_get_inode(req->r_dentry->d_sb, vino);
+		in = ceph_get_inode(req->r_dentry->d_sb, vino, NULL);
 		if (IS_ERR(in)) {
 			err = PTR_ERR(in);
 			dout("new_inode badness got %d\n", err);
@@ -1711,7 +1763,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		if (d_really_is_positive(dn)) {
 			in = d_inode(dn);
 		} else {
-			in = ceph_get_inode(parent->d_sb, tvino);
+			in = ceph_get_inode(parent->d_sb, tvino, NULL);
 			if (IS_ERR(in)) {
 				dout("new_inode badness\n");
 				d_drop(dn);
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 76d8d9495d1d..4107dc64cc8c 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -820,6 +820,7 @@ void ceph_mdsc_release_request(struct kref *kref)
 		ceph_async_iput(req->r_parent);
 	}
 	ceph_async_iput(req->r_target_inode);
+	ceph_async_iput(req->r_new_inode);
 	if (req->r_dentry)
 		dput(req->r_dentry);
 	if (req->r_old_dentry)
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 658800605bfb..63999f7db014 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -260,6 +260,7 @@ struct ceph_mds_request {
 
 	struct inode *r_parent;		    /* parent dir inode */
 	struct inode *r_target_inode;       /* resulting inode */
+	struct inode *r_new_inode;	    /* new inode (for creates) */
 
 #define CEPH_MDS_R_DIRECT_IS_HASH	(1) /* r_direct_hash is valid */
 #define CEPH_MDS_R_ABORTED		(2) /* call was aborted */
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 3b8ffa6aee46..d788fa9b3eaa 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -929,6 +929,7 @@ static inline bool __ceph_have_pending_cap_snap(struct ceph_inode_info *ci)
 /* inode.c */
 struct ceph_mds_reply_info_in;
 struct ceph_mds_reply_dirfrag;
+struct ceph_acl_sec_ctx;
 
 extern const struct inode_operations ceph_file_iops;
 
@@ -936,8 +937,10 @@ extern struct inode *ceph_alloc_inode(struct super_block *sb);
 extern void ceph_evict_inode(struct inode *inode);
 extern void ceph_free_inode(struct inode *inode);
 
+struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
+			     umode_t *mode, struct ceph_acl_sec_ctx *as_ctx);
 extern struct inode *ceph_get_inode(struct super_block *sb,
-				    struct ceph_vino vino);
+				    struct ceph_vino vino, struct inode *new);
 extern struct inode *ceph_get_snapdir(struct inode *parent);
 extern int ceph_fill_file_size(struct inode *inode, int issued,
 			       u32 truncate_seq, u64 truncate_size, u64 size);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (9 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 10/18] ceph: preallocate inode for ops that may create one Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  4:43   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr Jeff Layton
                   ` (7 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

After pre-creating a new inode, do an fscrypt prepare on it, fetch a
new encryption context and then marshal that into the security context
to be sent along with the RPC.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/crypto.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ceph/crypto.h |  8 ++++++
 fs/ceph/inode.c  | 10 ++++++--
 fs/ceph/super.h  |  3 +++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 22a09d422b72..f4849f8b32df 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -67,3 +67,66 @@ int ceph_fscrypt_set_ops(struct super_block *sb)
 	}
 	return 0;
 }
+
+int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
+				 struct ceph_acl_sec_ctx *as)
+{
+	int ret, ctxsize;
+	size_t name_len;
+	char *name;
+	struct ceph_pagelist *pagelist = as->pagelist;
+	bool encrypted = false;
+
+	ret = fscrypt_prepare_new_inode(dir, inode, &encrypted);
+	if (ret)
+		return ret;
+	if (!encrypted)
+		return 0;
+
+	inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
+
+	/* No need to set context for dummy encryption */
+	if (fscrypt_get_dummy_context(inode->i_sb))
+		return 0;
+
+	ctxsize = fscrypt_new_context_from_inode((union fscrypt_context *)&as->fscrypt, inode);
+
+	/* marshal it in page array */
+	if (!pagelist) {
+		pagelist = ceph_pagelist_alloc(GFP_KERNEL);
+		if (!pagelist)
+			return -ENOMEM;
+		ret = ceph_pagelist_reserve(pagelist, PAGE_SIZE);
+		if (ret)
+			goto out;
+		ceph_pagelist_encode_32(pagelist, 1);
+	}
+
+	name = CEPH_XATTR_NAME_ENCRYPTION_CONTEXT;
+	name_len = strlen(name);
+	ret = ceph_pagelist_reserve(pagelist, 4 * 2 + name_len + ctxsize);
+	if (ret)
+		goto out;
+
+	if (as->pagelist) {
+		BUG_ON(pagelist->length <= sizeof(__le32));
+		if (list_is_singular(&pagelist->head)) {
+			le32_add_cpu((__le32*)pagelist->mapped_tail, 1);
+		} else {
+			struct page *page = list_first_entry(&pagelist->head,
+							     struct page, lru);
+			void *addr = kmap_atomic(page);
+			le32_add_cpu((__le32*)addr, 1);
+			kunmap_atomic(addr);
+		}
+	}
+
+	ceph_pagelist_encode_32(pagelist, name_len);
+	ceph_pagelist_append(pagelist, name, name_len);
+	ceph_pagelist_encode_32(pagelist, ctxsize);
+	ceph_pagelist_append(pagelist, as->fscrypt, ctxsize);
+out:
+	if (pagelist && !as->pagelist)
+		ceph_pagelist_release(pagelist);
+	return ret;
+}
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index af06dca5f5a6..bf893bd215c3 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -11,6 +11,8 @@
 #define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"
 
 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 */
 
@@ -19,6 +21,12 @@ static inline int ceph_fscrypt_set_ops(struct super_block *sb)
 	return 0;
 }
 
+static inline int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
+						struct ceph_acl_sec_ctx *as)
+{
+	return 0;
+}
+
 #endif /* CONFIG_FS_ENCRYPTION */
 
 #endif
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e3c81b950f74..651148194316 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -18,6 +18,7 @@
 #include "super.h"
 #include "mds_client.h"
 #include "cache.h"
+#include "crypto.h"
 #include <linux/ceph/decode.h>
 
 /*
@@ -70,12 +71,17 @@ struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
 			goto out_err;
 	}
 
+	inode->i_state = 0;
+	inode->i_mode = *mode;
+
 	err = ceph_security_init_secctx(dentry, *mode, as_ctx);
 	if (err < 0)
 		goto out_err;
 
-	inode->i_state = 0;
-	inode->i_mode = *mode;
+	err = ceph_fscrypt_prepare_context(dir, inode, as_ctx);
+	if (err)
+		goto out_err;
+
 	return inode;
 out_err:
 	iput(inode);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index d788fa9b3eaa..cf04fcd3de69 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -995,6 +995,9 @@ struct ceph_acl_sec_ctx {
 #ifdef CONFIG_CEPH_FS_SECURITY_LABEL
 	void *sec_ctx;
 	u32 sec_ctxlen;
+#endif
+#ifdef CONFIG_FS_ENCRYPTION
+	u8	fscrypt[FSCRYPT_SET_CONTEXT_MAX_SIZE];
 #endif
 	struct ceph_pagelist *pagelist;
 };
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (10 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  4:57   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 13/18] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
                   ` (6 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

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(+)

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.
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 13/18] ceph: make ceph_msdc_build_path use ref-walk
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (11 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
                   ` (5 subsequent siblings)
  18 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Encryption potentially requires allocation, at which point we'll need to
be in a non-atomic context. Convert ceph_msdc_build_path to take dentry
spinlocks and references instead of using rcu_read_lock to walk the
path.

This is slightly less efficient, and we may want to eventually allow
using RCU when the leaf dentry isn't encrypted.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/mds_client.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 4107dc64cc8c..e3dc061252d4 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2327,7 +2327,8 @@ static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
 char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
 			   int stop_on_nosnap)
 {
-	struct dentry *temp;
+	struct dentry *cur;
+	struct inode *inode;
 	char *path;
 	int pos;
 	unsigned seq;
@@ -2344,34 +2345,35 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
 	path[pos] = '\0';
 
 	seq = read_seqbegin(&rename_lock);
-	rcu_read_lock();
-	temp = dentry;
+	cur = dget(dentry);
 	for (;;) {
-		struct inode *inode;
+		struct dentry *temp;
 
-		spin_lock(&temp->d_lock);
-		inode = d_inode(temp);
+		spin_lock(&cur->d_lock);
+		inode = d_inode(cur);
 		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
 			dout("build_path path+%d: %p SNAPDIR\n",
-			     pos, temp);
-		} else if (stop_on_nosnap && inode && dentry != temp &&
+			     pos, cur);
+		} else if (stop_on_nosnap && inode && dentry != cur &&
 			   ceph_snap(inode) == CEPH_NOSNAP) {
-			spin_unlock(&temp->d_lock);
+			spin_unlock(&cur->d_lock);
 			pos++; /* get rid of any prepended '/' */
 			break;
 		} else {
-			pos -= temp->d_name.len;
+			pos -= cur->d_name.len;
 			if (pos < 0) {
-				spin_unlock(&temp->d_lock);
+				spin_unlock(&cur->d_lock);
 				break;
 			}
-			memcpy(path + pos, temp->d_name.name, temp->d_name.len);
+			memcpy(path + pos, cur->d_name.name, cur->d_name.len);
 		}
+		temp = cur;
+		cur = dget(temp->d_parent);
 		spin_unlock(&temp->d_lock);
-		temp = READ_ONCE(temp->d_parent);
+		dput(temp);
 
 		/* Are we at the root? */
-		if (IS_ROOT(temp))
+		if (IS_ROOT(cur))
 			break;
 
 		/* Are we out of buffer? */
@@ -2380,8 +2382,9 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
 
 		path[pos] = '/';
 	}
-	base = ceph_ino(d_inode(temp));
-	rcu_read_unlock();
+	inode = d_inode(cur);
+	base = inode ? ceph_ino(inode) : 0;
+	dput(cur);
 
 	if (read_seqretry(&rename_lock, seq))
 		goto retry;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (12 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 13/18] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  5:06   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
                   ` (4 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Allow ceph_mdsc_build_path to encrypt and base64 encode the filename
when the parent is encrypted and we're sending the path to the MDS.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/mds_client.c | 51 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index e3dc061252d4..26b43ae09823 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -11,6 +11,7 @@
 #include <linux/ratelimit.h>
 #include <linux/bits.h>
 #include <linux/ktime.h>
+#include <linux/base64_fname.h>
 
 #include "super.h"
 #include "mds_client.h"
@@ -2324,8 +2325,7 @@ static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
  * Encode hidden .snap dirs as a double /, i.e.
  *   foo/.snap/bar -> foo//bar
  */
-char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
-			   int stop_on_nosnap)
+char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire)
 {
 	struct dentry *cur;
 	struct inode *inode;
@@ -2347,30 +2347,59 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
 	seq = read_seqbegin(&rename_lock);
 	cur = dget(dentry);
 	for (;;) {
-		struct dentry *temp;
+		struct dentry *parent;
 
 		spin_lock(&cur->d_lock);
 		inode = d_inode(cur);
+		parent = cur->d_parent;
 		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
 			dout("build_path path+%d: %p SNAPDIR\n",
 			     pos, cur);
-		} else if (stop_on_nosnap && inode && dentry != cur &&
-			   ceph_snap(inode) == CEPH_NOSNAP) {
+			dget(parent);
+			spin_unlock(&cur->d_lock);
+		} else if (for_wire && inode && dentry != cur && ceph_snap(inode) == CEPH_NOSNAP) {
 			spin_unlock(&cur->d_lock);
 			pos++; /* get rid of any prepended '/' */
 			break;
-		} else {
+		} else if (!for_wire || !IS_ENCRYPTED(d_inode(parent))) {
 			pos -= cur->d_name.len;
 			if (pos < 0) {
 				spin_unlock(&cur->d_lock);
 				break;
 			}
 			memcpy(path + pos, cur->d_name.name, cur->d_name.len);
+			dget(parent);
+			spin_unlock(&cur->d_lock);
+		} else {
+			int err;
+			struct fscrypt_name fname = { };
+			int len;
+			char buf[BASE64_CHARS(NAME_MAX)];
+
+			dget(parent);
+			spin_unlock(&cur->d_lock);
+
+			err = fscrypt_setup_filename(d_inode(parent), &cur->d_name, 1, &fname);
+			if (err) {
+				dput(parent);
+				dput(cur);
+				return ERR_PTR(err);
+			}
+
+			/* base64 encode the encrypted name */
+			len = base64_encode_fname(fname.disk_name.name, fname.disk_name.len, buf);
+			pos -= len;
+			if (pos < 0) {
+				dput(parent);
+				fscrypt_free_filename(&fname);
+				break;
+			}
+			memcpy(path + pos, buf, len);
+			dout("non-ciphertext name = %.*s\n", len, buf);
+			fscrypt_free_filename(&fname);
 		}
-		temp = cur;
-		cur = dget(temp->d_parent);
-		spin_unlock(&temp->d_lock);
-		dput(temp);
+		dput(cur);
+		cur = parent;
 
 		/* Are we at the root? */
 		if (IS_ROOT(cur))
@@ -2415,7 +2444,7 @@ static int build_dentry_path(struct dentry *dentry, struct inode *dir,
 	rcu_read_lock();
 	if (!dir)
 		dir = d_inode_rcu(dentry->d_parent);
-	if (dir && parent_locked && ceph_snap(dir) == CEPH_NOSNAP) {
+	if (dir && parent_locked && ceph_snap(dir) == CEPH_NOSNAP && !IS_ENCRYPTED(dir)) {
 		*pino = ceph_ino(dir);
 		rcu_read_unlock();
 		*ppath = dentry->d_name.name;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (13 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  5:12   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames Jeff Layton
                   ` (3 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

If we have an encrypted dentry, then we need to test whether a new key
might have been established or removed. Do that before we test anything
else about the dentry.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index b3f2741becdb..cc85933413b9 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1695,6 +1695,12 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 	dout("d_revalidate %p '%pd' inode %p offset 0x%llx\n", dentry,
 	     dentry, inode, ceph_dentry(dentry)->offset);
 
+	if (IS_ENCRYPTED(dir)) {
+		valid = fscrypt_d_revalidate(dentry, flags);
+		if (valid <= 0)
+			return valid;
+	}
+
 	mdsc = ceph_sb_to_client(dir->i_sb)->mdsc;
 
 	/* always trust cached snapped dentries, snapdir dentry */
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (14 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-08  5:34   ` Eric Biggers
  2020-09-04 16:05 ` [RFC PATCH v2 17/18] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
                   ` (2 subsequent siblings)
  18 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

Add helper functions for buffer management and for decrypting filenames
returned by the MDS. Wire those into the readdir codepaths.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/crypto.c | 47 +++++++++++++++++++++++++++++
 fs/ceph/crypto.h | 47 +++++++++++++++++++++++++++++
 fs/ceph/dir.c    | 77 ++++++++++++++++++++++++++++++++++++++++--------
 fs/ceph/inode.c  | 30 +++++++++++++++++--
 4 files changed, 186 insertions(+), 15 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index f4849f8b32df..8e6eb0ca0777 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -2,6 +2,7 @@
 #include <linux/ceph/ceph_debug.h>
 #include <linux/xattr.h>
 #include <linux/fscrypt.h>
+#include <linux/base64_fname.h>
 
 #include "super.h"
 #include "crypto.h"
@@ -130,3 +131,49 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
 		ceph_pagelist_release(pagelist);
 	return ret;
 }
+
+int ceph_fname_to_usr(struct inode *parent, char *name, u32 len,
+			struct fscrypt_str *tname, struct fscrypt_str *oname)
+{
+	int ret, declen;
+	u32 save_len;
+	struct fscrypt_str myname = FSTR_INIT(NULL, 0);
+
+	if (!IS_ENCRYPTED(parent)) {
+		oname->name = name;
+		oname->len = len;
+		return 0;
+	}
+
+	ret = fscrypt_get_encryption_info(parent);
+	if (ret)
+		return ret;
+
+	if (tname) {
+		save_len = tname->len;
+	} else {
+		int err;
+
+		save_len = 0;
+		err = fscrypt_fname_alloc_buffer(NAME_MAX, &myname);
+		if (err)
+			return err;
+		tname = &myname;
+	}
+
+	declen = base64_decode_fname(name, len, tname->name);
+	if (declen < 0 || declen > NAME_MAX) {
+		ret = -EIO;
+		goto out;
+	}
+
+	tname->len = declen;
+
+	ret = fscrypt_fname_disk_to_usr(parent, 0, 0, tname, oname);
+
+	if (save_len)
+		tname->len = save_len;
+out:
+	fscrypt_fname_free_buffer(&myname);
+	return ret;
+}
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index 1b11e9af165e..88c672ccdcf8 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -6,6 +6,8 @@
 #ifndef _CEPH_CRYPTO_H
 #define _CEPH_CRYPTO_H
 
+#include <linux/fscrypt.h>
+
 #ifdef CONFIG_FS_ENCRYPTION
 
 #define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"
@@ -16,6 +18,29 @@ 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);
 
+static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
+{
+	if (!IS_ENCRYPTED(parent))
+		return 0;
+	return fscrypt_fname_alloc_buffer(NAME_MAX, fname);
+}
+
+static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname)
+{
+	if (IS_ENCRYPTED(parent))
+		fscrypt_fname_free_buffer(fname);
+}
+
+static inline int ceph_get_encryption_info(struct inode *inode)
+{
+	if (!IS_ENCRYPTED(inode))
+		return 0;
+	return fscrypt_get_encryption_info(inode);
+}
+
+int ceph_fname_to_usr(struct inode *parent, char *name, u32 len,
+			struct fscrypt_str *tname, struct fscrypt_str *oname);
+
 #else /* CONFIG_FS_ENCRYPTION */
 
 #define DUMMY_ENCRYPTION_ENABLED(fsc) (0)
@@ -31,6 +56,28 @@ static inline int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *
 	return 0;
 }
 
+static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
+{
+	return 0;
+}
+
+static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname)
+{
+}
+
+static inline int ceph_get_encryption_info(struct inode *inode)
+{
+	return 0;
+}
+
+static inline int ceph_fname_to_usr(struct inode *inode, char *name, u32 len,
+			struct fscrypt_str *tname, struct fscrypt_str *oname)
+{
+	oname->name = dname;
+	oname->len = dlen;
+	return 0;
+}
+
 #endif /* CONFIG_FS_ENCRYPTION */
 
 #endif
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index cc85933413b9..0ba17c592fe1 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -6,9 +6,11 @@
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/xattr.h>
+#include <linux/base64_fname.h>
 
 #include "super.h"
 #include "mds_client.h"
+#include "crypto.h"
 
 /*
  * Directory operations: readdir, lookup, create, link, unlink,
@@ -168,6 +170,27 @@ __dcache_find_get_entry(struct dentry *parent, u64 idx,
 	return dentry ? : ERR_PTR(-EAGAIN);
 }
 
+static bool fscrypt_key_status_change(struct dentry *dentry)
+{
+	struct inode *dir;
+	bool encrypted_name, have_key;
+
+	lockdep_assert_held(&dentry->d_lock);
+
+	dir = d_inode(dentry->d_parent);
+	if (!IS_ENCRYPTED(dir))
+		return false;
+
+	encrypted_name = dentry->d_flags & DCACHE_ENCRYPTED_NAME;
+	have_key = fscrypt_has_encryption_key(dir);
+
+	if (encrypted_name == have_key)
+		ceph_dir_clear_complete(dir);
+
+	dout("%s encrypted_name=%d have_key=%d\n", __func__, encrypted_name, have_key);
+	return encrypted_name == have_key;
+}
+
 /*
  * When possible, we try to satisfy a readdir by peeking at the
  * dcache.  We make this work by carefully ordering dentries on
@@ -238,11 +261,11 @@ static int __dcache_readdir(struct file *file,  struct dir_context *ctx,
 			goto out;
 		}
 
-		spin_lock(&dentry->d_lock);
 		di = ceph_dentry(dentry);
 		if (d_unhashed(dentry) ||
 		    d_really_is_negative(dentry) ||
-		    di->lease_shared_gen != shared_gen) {
+		    di->lease_shared_gen != shared_gen ||
+		    fscrypt_key_status_change(dentry)) {
 			spin_unlock(&dentry->d_lock);
 			dput(dentry);
 			err = -EAGAIN;
@@ -314,6 +337,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 	int err;
 	unsigned frag = -1;
 	struct ceph_mds_reply_info_parsed *rinfo;
+	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
+	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
 
 	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
 	if (dfi->file_info.flags & CEPH_F_ATEND)
@@ -341,6 +366,10 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		ctx->pos = 2;
 	}
 
+	err = ceph_get_encryption_info(inode);
+	if (err)
+		goto out;
+
 	spin_lock(&ci->i_ceph_lock);
 	/* request Fx cap. if have Fx, we don't need to release Fs cap
 	 * for later create/unlink. */
@@ -361,6 +390,14 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		spin_unlock(&ci->i_ceph_lock);
 	}
 
+	err = ceph_fname_alloc_buffer(inode, &tname);
+	if (err < 0)
+		goto out;
+
+	err = ceph_fname_alloc_buffer(inode, &oname);
+	if (err < 0)
+		goto out;
+
 	/* proceed with a normal readdir */
 more:
 	/* do we have the correct frag content buffered? */
@@ -388,12 +425,14 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		dout("readdir fetching %llx.%llx frag %x offset '%s'\n",
 		     ceph_vinop(inode), frag, dfi->last_name);
 		req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
-		if (IS_ERR(req))
-			return PTR_ERR(req);
+		if (IS_ERR(req)) {
+			err = PTR_ERR(req);
+			goto out;
+		}
 		err = ceph_alloc_readdir_reply_buffer(req, inode);
 		if (err) {
 			ceph_mdsc_put_request(req);
-			return err;
+			goto out;
 		}
 		/* hints to request -> mds selection code */
 		req->r_direct_mode = USE_AUTH_MDS;
@@ -406,7 +445,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			req->r_path2 = kstrdup(dfi->last_name, GFP_KERNEL);
 			if (!req->r_path2) {
 				ceph_mdsc_put_request(req);
-				return -ENOMEM;
+				err = -ENOMEM;
+				goto out;
 			}
 		} else if (is_hash_order(ctx->pos)) {
 			req->r_args.readdir.offset_hash =
@@ -427,7 +467,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		err = ceph_mdsc_do_request(mdsc, NULL, req);
 		if (err < 0) {
 			ceph_mdsc_put_request(req);
-			return err;
+			goto out;
 		}
 		dout("readdir got and parsed readdir result=%d on "
 		     "frag %x, end=%d, complete=%d, hash_order=%d\n",
@@ -480,7 +520,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			err = note_last_dentry(dfi, rde->name, rde->name_len,
 					       next_offset);
 			if (err)
-				return err;
+				goto out;
 		} else if (req->r_reply_info.dir_end) {
 			dfi->next_offset = 2;
 			/* keep last name */
@@ -508,8 +548,10 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 	}
 	for (; i < rinfo->dir_nr; i++) {
 		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
+		u32 olen = oname.len;
 
 		BUG_ON(rde->offset < ctx->pos);
+		BUG_ON(!rde->inode.in);
 
 		ctx->pos = rde->offset;
 		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
@@ -518,12 +560,20 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 
 		BUG_ON(!rde->inode.in);
 
-		if (!dir_emit(ctx, rde->name, rde->name_len,
+		err = ceph_fname_to_usr(inode, rde->name, rde->name_len, &tname, &oname);
+		if (err)
+			goto out;
+
+		if (!dir_emit(ctx, oname.name, oname.len,
 			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
 			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
 			dout("filldir stopping us...\n");
-			return 0;
+			err = 0;
+			goto out;
 		}
+
+		/* Reset the lengths to their original allocated vals */
+		oname.len = olen;
 		ctx->pos++;
 	}
 
@@ -578,9 +628,12 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 					dfi->dir_ordered_count);
 		spin_unlock(&ci->i_ceph_lock);
 	}
-
+	err = 0;
 	dout("readdir %p file %p done.\n", inode, file);
-	return 0;
+out:
+	ceph_fname_free_buffer(inode, &tname);
+	ceph_fname_free_buffer(inode, &oname);
+	return err;
 }
 
 static void reset_readdir(struct ceph_dir_file_info *dfi)
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index c1c1fe2547f9..7d66f41a6592 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1648,7 +1648,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 			     struct ceph_mds_session *session)
 {
 	struct dentry *parent = req->r_dentry;
-	struct ceph_inode_info *ci = ceph_inode(d_inode(parent));
+	struct inode *inode = d_inode(parent);
+	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info;
 	struct qstr dname;
 	struct dentry *dn;
@@ -1659,6 +1660,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	u32 last_hash = 0;
 	u32 fpos_offset;
 	struct ceph_readdir_cache_control cache_ctl = {};
+	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
+	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
 
 	if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
 		return readdir_prepopulate_inodes_only(req, session);
@@ -1710,14 +1713,28 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	cache_ctl.index = req->r_readdir_cache_idx;
 	fpos_offset = req->r_readdir_offset;
 
+	err = ceph_fname_alloc_buffer(inode, &tname);
+	if (err < 0)
+		goto out;
+
+	err = ceph_fname_alloc_buffer(inode, &oname);
+	if (err < 0)
+		goto out;
+
 	/* FIXME: release caps/leases if error occurs */
 	for (i = 0; i < rinfo->dir_nr; i++) {
 		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
 		struct ceph_vino tvino;
+		u32 olen = oname.len;
 
-		dname.name = rde->name;
-		dname.len = rde->name_len;
+		err = ceph_fname_to_usr(inode, rde->name, rde->name_len, &tname, &oname);
+		if (err)
+			goto out;
+
+		dname.name = oname.name;
+		dname.len = oname.len;
 		dname.hash = full_name_hash(parent, dname.name, dname.len);
+		oname.len = olen;
 
 		tvino.ino = le64_to_cpu(rde->inode.in->ino);
 		tvino.snap = le64_to_cpu(rde->inode.in->snapid);
@@ -1748,6 +1765,11 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 				err = -ENOMEM;
 				goto out;
 			}
+			if (IS_ENCRYPTED(inode) && !fscrypt_has_encryption_key(inode)) {
+				spin_lock(&dn->d_lock);
+				dn->d_flags |= DCACHE_ENCRYPTED_NAME;
+				spin_unlock(&dn->d_lock);
+			}
 		} else if (d_really_is_positive(dn) &&
 			   (ceph_ino(d_inode(dn)) != tvino.ino ||
 			    ceph_snap(d_inode(dn)) != tvino.snap)) {
@@ -1838,6 +1860,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		req->r_readdir_cache_idx = cache_ctl.index;
 	}
 	ceph_readdir_cache_release(&cache_ctl);
+	ceph_fname_free_buffer(inode, &tname);
+	ceph_fname_free_buffer(inode, &oname);
 	dout("readdir_prepopulate done\n");
 	return err;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 17/18] ceph: add fscrypt support to ceph_fill_trace
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (15 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:05 ` [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
  2020-09-08  5:54 ` [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Eric Biggers
  18 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

When we get a dentry in a trace, decrypt the name so we can properly
instantiate the dentry.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/inode.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 7d66f41a6592..e578e4cdcf30 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1328,6 +1328,7 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 		    !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
 			struct qstr dname;
 			struct dentry *dn, *parent;
+			struct fscrypt_str oname = FSTR_INIT(NULL, 0);
 
 			BUG_ON(!rinfo->head->is_target);
 			BUG_ON(req->r_dentry);
@@ -1335,8 +1336,20 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 			parent = d_find_any_alias(dir);
 			BUG_ON(!parent);
 
-			dname.name = rinfo->dname;
-			dname.len = rinfo->dname_len;
+			err = ceph_fname_alloc_buffer(dir, &oname);
+			if (err < 0) {
+				dput(parent);
+				goto done;
+			}
+
+			err = ceph_fname_to_usr(dir, rinfo->dname, rinfo->dname_len, NULL, &oname);
+			if (err < 0) {
+				dput(parent);
+				ceph_fname_free_buffer(dir, &oname);
+				goto done;
+			}
+			dname.name = oname.name;
+			dname.len = oname.len;
 			dname.hash = full_name_hash(parent, dname.name, dname.len);
 			tvino.ino = le64_to_cpu(rinfo->targeti.in->ino);
 			tvino.snap = le64_to_cpu(rinfo->targeti.in->snapid);
@@ -1351,9 +1364,15 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 				     dname.len, dname.name, dn);
 				if (!dn) {
 					dput(parent);
+					ceph_fname_free_buffer(dir, &oname);
 					err = -ENOMEM;
 					goto done;
 				}
+				if (IS_ENCRYPTED(dir) && !fscrypt_has_encryption_key(dir)) {
+					spin_lock(&dn->d_lock);
+					dn->d_flags |= DCACHE_ENCRYPTED_NAME;
+					spin_unlock(&dn->d_lock);
+				}
 				err = 0;
 			} else if (d_really_is_positive(dn) &&
 				   (ceph_ino(d_inode(dn)) != tvino.ino ||
@@ -1365,6 +1384,7 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 				dput(dn);
 				goto retry_lookup;
 			}
+			ceph_fname_free_buffer(dir, &oname);
 
 			req->r_dentry = dn;
 			dput(parent);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (16 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 17/18] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
@ 2020-09-04 16:05 ` Jeff Layton
  2020-09-04 16:11   ` Jeff Layton
  2020-09-08  5:43   ` Eric Biggers
  2020-09-08  5:54 ` [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Eric Biggers
  18 siblings, 2 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:05 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

When creating symlinks in encrypted directories, encrypt and
base64-encode the target with the new inode's key before sending to the
MDS.

When filling a symlinked inode, base64-decode it into a buffer that
we'll keep in ci->i_symlink. When get_link is called, decrypt the buffer
into a new one that will hang off i_link.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c   | 32 ++++++++++++++++---
 fs/ceph/inode.c | 82 +++++++++++++++++++++++++++++++++++++++----------
 fs/ceph/super.h |  3 ++
 3 files changed, 96 insertions(+), 21 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 0ba17c592fe1..7ff8921dd3a7 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -948,6 +948,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 	struct ceph_mds_client *mdsc = fsc->mdsc;
 	struct ceph_mds_request *req;
 	struct ceph_acl_sec_ctx as_ctx = {};
+	struct fscrypt_str osd_link = FSTR_INIT(NULL, 0);
 	umode_t mode = S_IFLNK | 0777;
 	int err;
 
@@ -973,11 +974,33 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 		goto out_req;
 	}
 
-	req->r_path2 = kstrdup(dest, GFP_KERNEL);
-	if (!req->r_path2) {
-		err = -ENOMEM;
-		goto out_req;
+	if (IS_ENCRYPTED(req->r_new_inode)) {
+		int len = strlen(dest);
+
+		err = fscrypt_prepare_symlink(dir, dest, len, PATH_MAX, &osd_link);
+		if (err)
+			goto out_req;
+
+		err = fscrypt_encrypt_symlink(req->r_new_inode, dest, len, &osd_link);
+		if (err)
+			goto out_req;
+
+		req->r_path2 = kmalloc(BASE64_CHARS(osd_link.len) + 1, GFP_KERNEL);
+		if (!req->r_path2) {
+			err = -ENOMEM;
+			goto out_req;
+		}
+
+		len = base64_encode_fname(osd_link.name, osd_link.len, req->r_path2);
+		req->r_path2[len] = '\0';
+	} else {
+		req->r_path2 = kstrdup(dest, GFP_KERNEL);
+		if (!req->r_path2) {
+			err = -ENOMEM;
+			goto out_req;
+		}
 	}
+
 	req->r_parent = dir;
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
 	req->r_dentry = dget(dentry);
@@ -997,6 +1020,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 	if (err)
 		d_drop(dentry);
 	ceph_release_acl_sec_ctx(&as_ctx);
+	fscrypt_fname_free_buffer(&osd_link);
 	return err;
 }
 
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e578e4cdcf30..dd39f886b03c 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -14,6 +14,7 @@
 #include <linux/random.h>
 #include <linux/sort.h>
 #include <linux/iversion.h>
+#include <linux/base64_fname.h>
 
 #include "super.h"
 #include "mds_client.h"
@@ -33,8 +34,6 @@
  * (typically because they are in the message handler path).
  */
 
-static const struct inode_operations ceph_symlink_iops;
-
 static void ceph_inode_work(struct work_struct *work);
 
 /*
@@ -503,6 +502,7 @@ struct inode *ceph_alloc_inode(struct super_block *sb)
 	atomic64_set(&ci->i_complete_seq[0], 0);
 	atomic64_set(&ci->i_complete_seq[1], 0);
 	ci->i_symlink = NULL;
+	ci->i_symlink_len = 0;
 
 	ci->i_max_bytes = 0;
 	ci->i_max_files = 0;
@@ -590,6 +590,7 @@ void ceph_free_inode(struct inode *inode)
 	struct ceph_inode_info *ci = ceph_inode(inode);
 
 	kfree(ci->i_symlink);
+	fscrypt_free_inode(inode);
 	kmem_cache_free(ceph_inode_cachep, ci);
 }
 
@@ -991,34 +992,60 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
 		inode->i_fop = &ceph_file_fops;
 		break;
 	case S_IFLNK:
-		inode->i_op = &ceph_symlink_iops;
 		if (!ci->i_symlink) {
 			u32 symlen = iinfo->symlink_len;
 			char *sym;
 
 			spin_unlock(&ci->i_ceph_lock);
 
-			if (symlen != i_size_read(inode)) {
-				pr_err("%s %llx.%llx BAD symlink "
-					"size %lld\n", __func__,
-					ceph_vinop(inode),
-					i_size_read(inode));
+			if (IS_ENCRYPTED(inode)) {
+				/* Do base64 decode so that we get the right size (maybe?) */
+				err = -ENOMEM;
+				sym = kmalloc(symlen + 1, GFP_NOFS);
+				if (!sym)
+					goto out;
+
+				symlen = base64_decode_fname(iinfo->symlink, symlen, sym);
+				/*
+				 * i_size as reported by the MDS may be wrong, due to base64
+				 * inflation and padding. Fix it up here.
+				 */
 				i_size_write(inode, symlen);
 				inode->i_blocks = calc_inode_blocks(symlen);
-			}
+			} else {
+				if (symlen != i_size_read(inode)) {
+					pr_err("%s %llx.%llx BAD symlink size %lld\n",
+						__func__, ceph_vinop(inode), i_size_read(inode));
+					i_size_write(inode, symlen);
+					inode->i_blocks = calc_inode_blocks(symlen);
+				}
 
-			err = -ENOMEM;
-			sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
-			if (!sym)
-				goto out;
+				err = -ENOMEM;
+				sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
+				if (!sym)
+					goto out;
+			}
 
 			spin_lock(&ci->i_ceph_lock);
-			if (!ci->i_symlink)
+			if (!ci->i_symlink) {
 				ci->i_symlink = sym;
-			else
+				ci->i_symlink_len = symlen;
+			} else {
 				kfree(sym); /* lost a race */
+			}
+		}
+
+		if (IS_ENCRYPTED(inode)) {
+			/*
+			 * Encrypted symlinks need to be decrypted before we can
+			 * cache their targets in i_link. Leave it blank for now.
+			 */
+			inode->i_link = NULL;
+			inode->i_op = &ceph_encrypted_symlink_iops;
+		} else {
+			inode->i_link = ci->i_symlink;
+			inode->i_op = &ceph_symlink_iops;
 		}
-		inode->i_link = ci->i_symlink;
 		break;
 	case S_IFDIR:
 		inode->i_op = &ceph_dir_iops;
@@ -2123,16 +2150,37 @@ static void ceph_inode_work(struct work_struct *work)
 	iput(inode);
 }
 
+static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
+					   struct delayed_call *done)
+{
+	struct ceph_inode_info *ci = ceph_inode(inode);
+
+	if (inode->i_link)
+		return inode->i_link;
+
+	if (!dentry)
+		return ERR_PTR(-ECHILD);
+
+	return fscrypt_get_symlink(inode, ci->i_symlink, ci->i_symlink_len, done);
+}
+
 /*
  * symlinks
  */
-static const struct inode_operations ceph_symlink_iops = {
+const struct inode_operations ceph_symlink_iops = {
 	.get_link = simple_get_link,
 	.setattr = ceph_setattr,
 	.getattr = ceph_getattr,
 	.listxattr = ceph_listxattr,
 };
 
+const struct inode_operations ceph_encrypted_symlink_iops = {
+	.get_link = ceph_encrypted_get_link,
+	.setattr = ceph_setattr,
+	.getattr = ceph_getattr,
+	.listxattr = ceph_listxattr,
+};
+
 int __ceph_setattr(struct inode *inode, struct iattr *attr)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 7c859824f64c..eea0ee17b747 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -344,6 +344,7 @@ struct ceph_inode_info {
 	u64 i_max_bytes, i_max_files;
 
 	s32 i_dir_pin;
+	u32 i_symlink_len;
 
 	struct rb_root i_fragtree;
 	int i_fragtree_nsplits;
@@ -932,6 +933,8 @@ struct ceph_mds_reply_dirfrag;
 struct ceph_acl_sec_ctx;
 
 extern const struct inode_operations ceph_file_iops;
+extern const struct inode_operations ceph_symlink_iops;
+extern const struct inode_operations ceph_encrypted_symlink_iops;
 
 extern struct inode *ceph_alloc_inode(struct super_block *sb);
 extern void ceph_evict_inode(struct inode *inode);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets
  2020-09-04 16:05 ` [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
@ 2020-09-04 16:11   ` Jeff Layton
  2020-09-08  5:43   ` Eric Biggers
  1 sibling, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-04 16:11 UTC (permalink / raw)
  To: ceph-devel; +Cc: linux-fsdevel, linux-fscrypt, ebiggers

On Fri, 2020-09-04 at 12:05 -0400, Jeff Layton wrote:
> When creating symlinks in encrypted directories, encrypt and
> base64-encode the target with the new inode's key before sending to the
> MDS.
> 
> When filling a symlinked inode, base64-decode it into a buffer that
> we'll keep in ci->i_symlink. When get_link is called, decrypt the buffer
> into a new one that will hang off i_link.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/dir.c   | 32 ++++++++++++++++---
>  fs/ceph/inode.c | 82 +++++++++++++++++++++++++++++++++++++++----------
>  fs/ceph/super.h |  3 ++
>  3 files changed, 96 insertions(+), 21 deletions(-)
> 

FWIW, this patch is not working right, and I'm not sure why.

I've validated that I'm getting the same crypttext out
of fscrypt_encrypt_symlink that I'm feeding into fscrypt_get_symlink,
but the contents always look like gibberish in a readlink.

Did I make an obvious mistake somewhere?


> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 0ba17c592fe1..7ff8921dd3a7 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -948,6 +948,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
>  	struct ceph_mds_client *mdsc = fsc->mdsc;
>  	struct ceph_mds_request *req;
>  	struct ceph_acl_sec_ctx as_ctx = {};
> +	struct fscrypt_str osd_link = FSTR_INIT(NULL, 0);
>  	umode_t mode = S_IFLNK | 0777;
>  	int err;
>  
> @@ -973,11 +974,33 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
>  		goto out_req;
>  	}
>  
> -	req->r_path2 = kstrdup(dest, GFP_KERNEL);
> -	if (!req->r_path2) {
> -		err = -ENOMEM;
> -		goto out_req;
> +	if (IS_ENCRYPTED(req->r_new_inode)) {
> +		int len = strlen(dest);
> +
> +		err = fscrypt_prepare_symlink(dir, dest, len, PATH_MAX, &osd_link);
> +		if (err)
> +			goto out_req;
> +
> +		err = fscrypt_encrypt_symlink(req->r_new_inode, dest, len, &osd_link);
> +		if (err)
> +			goto out_req;
> +
> +		req->r_path2 = kmalloc(BASE64_CHARS(osd_link.len) + 1, GFP_KERNEL);
> +		if (!req->r_path2) {
> +			err = -ENOMEM;
> +			goto out_req;
> +		}
> +
> +		len = base64_encode_fname(osd_link.name, osd_link.len, req->r_path2);
> +		req->r_path2[len] = '\0';
> +	} else {
> +		req->r_path2 = kstrdup(dest, GFP_KERNEL);
> +		if (!req->r_path2) {
> +			err = -ENOMEM;
> +			goto out_req;
> +		}
>  	}
> +
>  	req->r_parent = dir;
>  	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
>  	req->r_dentry = dget(dentry);
> @@ -997,6 +1020,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
>  	if (err)
>  		d_drop(dentry);
>  	ceph_release_acl_sec_ctx(&as_ctx);
> +	fscrypt_fname_free_buffer(&osd_link);
>  	return err;
>  }
>  
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index e578e4cdcf30..dd39f886b03c 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -14,6 +14,7 @@
>  #include <linux/random.h>
>  #include <linux/sort.h>
>  #include <linux/iversion.h>
> +#include <linux/base64_fname.h>
>  
>  #include "super.h"
>  #include "mds_client.h"
> @@ -33,8 +34,6 @@
>   * (typically because they are in the message handler path).
>   */
>  
> -static const struct inode_operations ceph_symlink_iops;
> -
>  static void ceph_inode_work(struct work_struct *work);
>  
>  /*
> @@ -503,6 +502,7 @@ struct inode *ceph_alloc_inode(struct super_block *sb)
>  	atomic64_set(&ci->i_complete_seq[0], 0);
>  	atomic64_set(&ci->i_complete_seq[1], 0);
>  	ci->i_symlink = NULL;
> +	ci->i_symlink_len = 0;
>  
>  	ci->i_max_bytes = 0;
>  	ci->i_max_files = 0;
> @@ -590,6 +590,7 @@ void ceph_free_inode(struct inode *inode)
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  
>  	kfree(ci->i_symlink);
> +	fscrypt_free_inode(inode);
>  	kmem_cache_free(ceph_inode_cachep, ci);
>  }
>  
> @@ -991,34 +992,60 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
>  		inode->i_fop = &ceph_file_fops;
>  		break;
>  	case S_IFLNK:
> -		inode->i_op = &ceph_symlink_iops;
>  		if (!ci->i_symlink) {
>  			u32 symlen = iinfo->symlink_len;
>  			char *sym;
>  
>  			spin_unlock(&ci->i_ceph_lock);
>  
> -			if (symlen != i_size_read(inode)) {
> -				pr_err("%s %llx.%llx BAD symlink "
> -					"size %lld\n", __func__,
> -					ceph_vinop(inode),
> -					i_size_read(inode));
> +			if (IS_ENCRYPTED(inode)) {
> +				/* Do base64 decode so that we get the right size (maybe?) */
> +				err = -ENOMEM;
> +				sym = kmalloc(symlen + 1, GFP_NOFS);
> +				if (!sym)
> +					goto out;
> +
> +				symlen = base64_decode_fname(iinfo->symlink, symlen, sym);
> +				/*
> +				 * i_size as reported by the MDS may be wrong, due to base64
> +				 * inflation and padding. Fix it up here.
> +				 */
>  				i_size_write(inode, symlen);
>  				inode->i_blocks = calc_inode_blocks(symlen);
> -			}
> +			} else {
> +				if (symlen != i_size_read(inode)) {
> +					pr_err("%s %llx.%llx BAD symlink size %lld\n",
> +						__func__, ceph_vinop(inode), i_size_read(inode));
> +					i_size_write(inode, symlen);
> +					inode->i_blocks = calc_inode_blocks(symlen);
> +				}
>  
> -			err = -ENOMEM;
> -			sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
> -			if (!sym)
> -				goto out;
> +				err = -ENOMEM;
> +				sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
> +				if (!sym)
> +					goto out;
> +			}
>  
>  			spin_lock(&ci->i_ceph_lock);
> -			if (!ci->i_symlink)
> +			if (!ci->i_symlink) {
>  				ci->i_symlink = sym;
> -			else
> +				ci->i_symlink_len = symlen;
> +			} else {
>  				kfree(sym); /* lost a race */
> +			}
> +		}
> +
> +		if (IS_ENCRYPTED(inode)) {
> +			/*
> +			 * Encrypted symlinks need to be decrypted before we can
> +			 * cache their targets in i_link. Leave it blank for now.
> +			 */
> +			inode->i_link = NULL;
> +			inode->i_op = &ceph_encrypted_symlink_iops;
> +		} else {
> +			inode->i_link = ci->i_symlink;
> +			inode->i_op = &ceph_symlink_iops;
>  		}
> -		inode->i_link = ci->i_symlink;
>  		break;
>  	case S_IFDIR:
>  		inode->i_op = &ceph_dir_iops;
> @@ -2123,16 +2150,37 @@ static void ceph_inode_work(struct work_struct *work)
>  	iput(inode);
>  }
>  
> +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> +					   struct delayed_call *done)
> +{
> +	struct ceph_inode_info *ci = ceph_inode(inode);
> +
> +	if (inode->i_link)
> +		return inode->i_link;
> +
> +	if (!dentry)
> +		return ERR_PTR(-ECHILD);
> +
> +	return fscrypt_get_symlink(inode, ci->i_symlink, ci->i_symlink_len, done);
> +}
> +
>  /*
>   * symlinks
>   */
> -static const struct inode_operations ceph_symlink_iops = {
> +const struct inode_operations ceph_symlink_iops = {
>  	.get_link = simple_get_link,
>  	.setattr = ceph_setattr,
>  	.getattr = ceph_getattr,
>  	.listxattr = ceph_listxattr,
>  };
>  
> +const struct inode_operations ceph_encrypted_symlink_iops = {
> +	.get_link = ceph_encrypted_get_link,
> +	.setattr = ceph_setattr,
> +	.getattr = ceph_getattr,
> +	.listxattr = ceph_listxattr,
> +};
> +
>  int __ceph_setattr(struct inode *inode, struct iattr *attr)
>  {
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 7c859824f64c..eea0ee17b747 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -344,6 +344,7 @@ struct ceph_inode_info {
>  	u64 i_max_bytes, i_max_files;
>  
>  	s32 i_dir_pin;
> +	u32 i_symlink_len;
>  
>  	struct rb_root i_fragtree;
>  	int i_fragtree_nsplits;
> @@ -932,6 +933,8 @@ struct ceph_mds_reply_dirfrag;
>  struct ceph_acl_sec_ctx;
>  
>  extern const struct inode_operations ceph_file_iops;
> +extern const struct inode_operations ceph_symlink_iops;
> +extern const struct inode_operations ceph_encrypted_symlink_iops;
>  
>  extern struct inode *ceph_alloc_inode(struct super_block *sb);
>  extern void ceph_evict_inode(struct inode *inode);

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-04 16:05 ` [RFC PATCH v2 01/18] vfs: export new_inode_pseudo Jeff Layton
@ 2020-09-08  3:38   ` Eric Biggers
  2020-09-08 11:27     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  3:38 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:20PM -0400, Jeff Layton wrote:
> Ceph needs to be able to allocate inodes ahead of a create that might
> involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
> but it puts the inode on the sb->s_inodes list, and we don't want to
> do that until we're ready to insert it into the hash.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/inode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 72c4c347afb7..61554c2477ab 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
>  	}
>  	return inode;
>  }
> +EXPORT_SYMBOL(new_inode_pseudo);
>  

What's the problem with putting the new inode on sb->s_inodes already?
That's what all the other filesystems do.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode
  2020-09-04 16:05 ` [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode Jeff Layton
@ 2020-09-08  3:48   ` Eric Biggers
  2020-09-08 11:29     ` Jeff Layton
  2020-09-08 12:29     ` Jeff Layton
  0 siblings, 2 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  3:48 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:23PM -0400, Jeff Layton wrote:
> CephFS will need to be able to generate a context for a new "prepared"
> inode. Add a new routine for getting the context out of an in-core
> inode.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/policy.c      | 20 ++++++++++++++++++++
>  include/linux/fscrypt.h |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index c56ad886f7d7..10eddd113a21 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_set_context);
>  
> +/**
> + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode

Comment doesn't match the function name.

Also, the name isn't very clear.  How about calling this
fscrypt_context_for_new_inode()?

BTW, I might rename fscrypt_new_context_from_policy() to
fscrypt_context_from_policy() in my patchset.  Since it now makes the caller
provide the nonce, technically it's no longer limited to "new" contexts.

> + * @ctx: where context should be written
> + * @inode: inode from which to fetch context
> + *
> + * Given an in-core prepared, but not-necessarily fully-instantiated inode,
> + * generate an encryption context from its policy and write it to ctx.

Clarify what is meant by "prepared" (fscrypt_prepare_new_inode() was called)
vs. "instantiated".

> + *
> + * Returns size of the context.
> + */
> +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode)
> +{
> +	struct fscrypt_info *ci = inode->i_crypt_info;
> +
> +	BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
> +
> +	return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce);
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode);

fscrypt_set_context() should be changed to call this, instead of duplicating the
same logic.  As part of that, the WARN_ON_ONCE(!ci) that's currently in
fscrypt_set_context() should go in here instead.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted
  2020-09-04 16:05 ` [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted Jeff Layton
@ 2020-09-08  3:52   ` Eric Biggers
  2020-09-08 12:54     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  3:52 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:24PM -0400, Jeff Layton wrote:
> Cephfs (currently) sets this flag early and only fetches the context
> later. Eventually we may not need this, but for now it prevents this
> warning from popping.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/keysetup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index ad64525ec680..3b4ec16fc528 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
>  		const union fscrypt_context *dummy_ctx =
>  			fscrypt_get_dummy_context(inode->i_sb);
>  
> -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> +		if (!dummy_ctx) {
>  			fscrypt_warn(inode,
>  				     "Error %d getting encryption context",
>  				     res);

This makes errors reading the encryption xattr of an encrypted inode be ignored
when the filesystem is mounted with test_dummy_encryption.  That's undesirable.

Isn't this change actually no longer needed, now that new inodes will use
fscrypt_prepare_new_inode() instead of fscrypt_get_encryption_info()?

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it
  2020-09-04 16:05 ` [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it Jeff Layton
@ 2020-09-08  3:55   ` Eric Biggers
  2020-09-08 12:50     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  3:55 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:25PM -0400, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/fname.c       | 71 +++++++++++++++++++++++------------------
>  include/linux/fscrypt.h |  3 ++
>  2 files changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 9440a44e24ac..09f09def87fc 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -300,6 +300,45 @@ void fscrypt_fname_free_buffer(struct fscrypt_str *crypto_str)
>  }
>  EXPORT_SYMBOL(fscrypt_fname_free_buffer);
>  
> +void fscrypt_encode_nokey_name(u32 hash, u32 minor_hash,
> +			     const struct fscrypt_str *iname,
> +			     struct fscrypt_str *oname)
> +{
> +	struct fscrypt_nokey_name nokey_name;
> +	u32 size; /* size of the unencoded no-key name */
> +
> +	/*
> +	 * Sanity check that struct fscrypt_nokey_name doesn't have padding
> +	 * between fields and that its encoded size never exceeds NAME_MAX.
> +	 */
> +	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, dirhash) !=
> +		     offsetof(struct fscrypt_nokey_name, bytes));
> +	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, bytes) !=
> +		     offsetof(struct fscrypt_nokey_name, sha256));
> +	BUILD_BUG_ON(BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX) > NAME_MAX);
> +
> +	if (hash) {
> +		nokey_name.dirhash[0] = hash;
> +		nokey_name.dirhash[1] = minor_hash;
> +	} else {
> +		nokey_name.dirhash[0] = 0;
> +		nokey_name.dirhash[1] = 0;
> +	}
> +	if (iname->len <= sizeof(nokey_name.bytes)) {
> +		memcpy(nokey_name.bytes, iname->name, iname->len);
> +		size = offsetof(struct fscrypt_nokey_name, bytes[iname->len]);
> +	} else {
> +		memcpy(nokey_name.bytes, iname->name, sizeof(nokey_name.bytes));
> +		/* Compute strong hash of remaining part of name. */
> +		fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)],
> +				  iname->len - sizeof(nokey_name.bytes),
> +				  nokey_name.sha256);
> +		size = FSCRYPT_NOKEY_NAME_MAX;
> +	}
> +	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
> +}
> +EXPORT_SYMBOL(fscrypt_encode_nokey_name);

Why does this need to be exported?

There's no user of this function introduced in this patchset.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/
  2020-09-04 16:05 ` [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/ Jeff Layton
@ 2020-09-08  3:59   ` Eric Biggers
  2020-09-08 12:51     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  3:59 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:26PM -0400, Jeff Layton wrote:
> Once we allow encrypted filenames on ceph we'll end up with names that
> may have illegal characters in them (embedded '\0' or '/'), or
> characters that aren't printable.
> 
> It will be safer to use strings that are printable. It turns out that the
> MDS doesn't really care about the length of filenames, so we can just
> base64 encode and decode filenames before writing and reading them.
> 
> Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
> select it when it's enabled.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/Kconfig            |  1 +
>  fs/crypto/fname.c            | 64 ++------------------------------
>  include/linux/base64_fname.h | 11 ++++++
>  lib/Kconfig                  |  3 ++
>  lib/Makefile                 |  1 +
>  lib/base64_fname.c           | 71 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 90 insertions(+), 61 deletions(-)
>  create mode 100644 include/linux/base64_fname.h
>  create mode 100644 lib/base64_fname.c
> 

I'm still concerned that this functionality is too specific to belong in lib/ at
the moment, given that it's not the most commonly used variant of base64.  How
about keeping these functions in fs/crypto/ for now?  You can call them
fscrypt_base64_encode() and fscrypt_base64_decode() and export them for ceph to
use.

> diff --git a/lib/base64_fname.c b/lib/base64_fname.c
> new file mode 100644
> index 000000000000..7638c45e4035
> --- /dev/null
> +++ b/lib/base64_fname.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Modified base64 encode/decode functions, suitable for use as filename components.
> + *
> + * Originally lifted from fs/crypto/fname.c
> + *
> + * Copyright (C) 2015, Jaegeuk Kim
> + * Copyright (C) 2015, Eric Biggers
> + */

Please don't change the copyright statements.  The original file had:

 * Copyright (C) 2015, Google, Inc.
 * Copyright (C) 2015, Motorola Mobility

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 09/18] ceph: crypto context handling for ceph
  2020-09-04 16:05 ` [RFC PATCH v2 09/18] ceph: crypto context handling for ceph Jeff Layton
@ 2020-09-08  4:29   ` Eric Biggers
  2020-09-08 16:14     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  4:29 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:28PM -0400, Jeff Layton wrote:
> Store the fscrypt context for an inode as an encryption.ctx xattr.
> 
> Also add support for "dummy" encryption (useful for testing with
> automated test harnesses like xfstests).

Can you put the test_dummy_encryption support in a separate patch?

> +static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> +{
> +	int ret = __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);
> +
> +	if (ret > 0)
> +		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
> +	return ret;
> +}
> +
> +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;
> +}

get_context() shouldn't be setting the S_ENCRYPTED inode flag.
Only set_context() should be doing that.

> +
> +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 const union fscrypt_context *
> +ceph_get_dummy_context(struct super_block *sb)
> +{
> +	return ceph_sb_to_client(sb)->dummy_enc_ctx.ctx;
> +}
> +
> +static struct fscrypt_operations ceph_fscrypt_ops = {
> +	.key_prefix		= "ceph:",

IMO you shouldn't set .key_prefix here, since it's deprecated.
Just leave it unset so that ceph will only support the generic prefix "fscrypt:"
as well as FS_IOC_ADD_ENCRYPTION_KEY.

>  enum ceph_recover_session_mode {
> @@ -197,6 +200,8 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
>  	fsparam_u32	("rsize",			Opt_rsize),
>  	fsparam_string	("snapdirname",			Opt_snapdirname),
>  	fsparam_string	("source",			Opt_source),
> +	fsparam_flag_no ("test_dummy_encryption",	Opt_test_dummy_encryption),
> +	fsparam_string	("test_dummy_encryption",	Opt_test_dummy_encryption),

I think you should use fsparam_flag instead of fsparam_flag_no, since otherwise
"notest_dummy_encryption" will be recognized.  There's not a problem with it
per se, but none of the other filesystems that support "test_dummy_encryption"
allow "notest_dummy_encryption".  It's nice to keep things consistent.

I.e. if "notest_dummy_encryption" is really something that would be useful
(presumably only for remount, since it's a test-only option that will never be
on by default), then we should add it to ext4, f2fs, and ceph -- not just ceph.

> +	/* Don't allow test_dummy_encryption to change on remount */
> +	if (fsopt->flags & CEPH_MOUNT_OPT_TEST_DUMMY_ENC) {
> +		if (!ceph_test_mount_opt(fsc, TEST_DUMMY_ENC))
> +			return -EEXIST;
> +	} else {
> +		if (ceph_test_mount_opt(fsc, TEST_DUMMY_ENC))
> +			return -EEXIST;
> +	}
> +

Can you check what ext4 and f2fs do for this?  test_dummy_encryption isn't just
a boolean flag anymore, so this logic isn't sufficient to prevent it from
changing during remount.  For example someone could mount with
test_dummy_encryption=v1, then try to remount with test_dummy_encryption=v2.
On ext4 and f2fs, that intentionally fails.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC
  2020-09-04 16:05 ` [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC Jeff Layton
@ 2020-09-08  4:43   ` Eric Biggers
  0 siblings, 0 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  4:43 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:30PM -0400, Jeff Layton wrote:
> After pre-creating a new inode, do an fscrypt prepare on it, fetch a
> new encryption context and then marshal that into the security context
> to be sent along with the RPC.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/crypto.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ceph/crypto.h |  8 ++++++
>  fs/ceph/inode.c  | 10 ++++++--
>  fs/ceph/super.h  |  3 +++
>  4 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 22a09d422b72..f4849f8b32df 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -67,3 +67,66 @@ int ceph_fscrypt_set_ops(struct super_block *sb)
>  	}
>  	return 0;
>  }
> +
> +int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
> +				 struct ceph_acl_sec_ctx *as)
> +{
> +	int ret, ctxsize;
> +	size_t name_len;
> +	char *name;
> +	struct ceph_pagelist *pagelist = as->pagelist;
> +	bool encrypted = false;
> +
> +	ret = fscrypt_prepare_new_inode(dir, inode, &encrypted);
> +	if (ret)
> +		return ret;
> +	if (!encrypted)
> +		return 0;
> +
> +	inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);

This is a new inode, so 'inode->i_flags |= S_ENCRYPTED' would be sufficient.

> +
> +	/* No need to set context for dummy encryption */
> +	if (fscrypt_get_dummy_context(inode->i_sb))
> +		return 0;

This isn't correct.  When test_dummy_encryption causes a new inode to be
automatically encrypted, the inode's encryption context is still supposed to be
saved to disk.

Also, when the filesystem is mounted with test_dummy_encryption, there may
already be existing encrypted directories that were created via the regular path
(not via test_dummy_encryption).  Those should keep working as expected.  That
likewise requires saving new encryption contexts to disk.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  2020-09-04 16:05 ` [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr Jeff Layton
@ 2020-09-08  4:57   ` Eric Biggers
  2020-09-09 12:20     ` Jeff Layton
  2020-09-09 15:53     ` Jeff Layton
  0 siblings, 2 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  4:57 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

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

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path
  2020-09-04 16:05 ` [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
@ 2020-09-08  5:06   ` Eric Biggers
  2020-09-09 12:24     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  5:06 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:33PM -0400, Jeff Layton wrote:
> Allow ceph_mdsc_build_path to encrypt and base64 encode the filename
> when the parent is encrypted and we're sending the path to the MDS.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/mds_client.c | 51 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index e3dc061252d4..26b43ae09823 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -11,6 +11,7 @@
>  #include <linux/ratelimit.h>
>  #include <linux/bits.h>
>  #include <linux/ktime.h>
> +#include <linux/base64_fname.h>
>  
>  #include "super.h"
>  #include "mds_client.h"
> @@ -2324,8 +2325,7 @@ static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
>   * Encode hidden .snap dirs as a double /, i.e.
>   *   foo/.snap/bar -> foo//bar
>   */
> -char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> -			   int stop_on_nosnap)
> +char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire)
>  {
>  	struct dentry *cur;
>  	struct inode *inode;
> @@ -2347,30 +2347,59 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
>  	seq = read_seqbegin(&rename_lock);
>  	cur = dget(dentry);
>  	for (;;) {
> -		struct dentry *temp;
> +		struct dentry *parent;
>  
>  		spin_lock(&cur->d_lock);
>  		inode = d_inode(cur);
> +		parent = cur->d_parent;
>  		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
>  			dout("build_path path+%d: %p SNAPDIR\n",
>  			     pos, cur);
> -		} else if (stop_on_nosnap && inode && dentry != cur &&
> -			   ceph_snap(inode) == CEPH_NOSNAP) {
> +			dget(parent);
> +			spin_unlock(&cur->d_lock);
> +		} else if (for_wire && inode && dentry != cur && ceph_snap(inode) == CEPH_NOSNAP) {
>  			spin_unlock(&cur->d_lock);
>  			pos++; /* get rid of any prepended '/' */
>  			break;
> -		} else {
> +		} else if (!for_wire || !IS_ENCRYPTED(d_inode(parent))) {
>  			pos -= cur->d_name.len;
>  			if (pos < 0) {
>  				spin_unlock(&cur->d_lock);
>  				break;
>  			}
>  			memcpy(path + pos, cur->d_name.name, cur->d_name.len);
> +			dget(parent);
> +			spin_unlock(&cur->d_lock);
> +		} else {
> +			int err;
> +			struct fscrypt_name fname = { };
> +			int len;
> +			char buf[BASE64_CHARS(NAME_MAX)];
> +
> +			dget(parent);
> +			spin_unlock(&cur->d_lock);
> +
> +			err = fscrypt_setup_filename(d_inode(parent), &cur->d_name, 1, &fname);

How are no-key filenames handled with ceph?  You're calling
fscrypt_setup_filename() with lookup=1, so it will give you back a no-key
filename if the directory's encryption key is unavailable.

> +			if (err) {
> +				dput(parent);
> +				dput(cur);
> +				return ERR_PTR(err);
> +			}
> +
> +			/* base64 encode the encrypted name */
> +			len = base64_encode_fname(fname.disk_name.name, fname.disk_name.len, buf);
> +			pos -= len;
> +			if (pos < 0) {
> +				dput(parent);
> +				fscrypt_free_filename(&fname);
> +				break;
> +			}
> +			memcpy(path + pos, buf, len);
> +			dout("non-ciphertext name = %.*s\n", len, buf);
> +			fscrypt_free_filename(&fname);

This would be easier to understand if the encryption and encoding logic was
moved into its own function.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries
  2020-09-04 16:05 ` [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
@ 2020-09-08  5:12   ` Eric Biggers
  2020-09-09 12:26     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  5:12 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:34PM -0400, Jeff Layton wrote:
> If we have an encrypted dentry, then we need to test whether a new key
> might have been established or removed. Do that before we test anything
> else about the dentry.

A more accurate explanation would be:

"If we have a dentry which represents a no-key name, then we need to test
 whether the parent directory's encryption key has since been added."

> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/dir.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index b3f2741becdb..cc85933413b9 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1695,6 +1695,12 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
>  	dout("d_revalidate %p '%pd' inode %p offset 0x%llx\n", dentry,
>  	     dentry, inode, ceph_dentry(dentry)->offset);
>  
> +	if (IS_ENCRYPTED(dir)) {
> +		valid = fscrypt_d_revalidate(dentry, flags);
> +		if (valid <= 0)
> +			return valid;
> +	}

There's no need to check IS_ENCRYPTED(dir) here.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames
  2020-09-04 16:05 ` [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames Jeff Layton
@ 2020-09-08  5:34   ` Eric Biggers
  2020-09-09 13:02     ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  5:34 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:35PM -0400, Jeff Layton wrote:
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index 1b11e9af165e..88c672ccdcf8 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -6,6 +6,8 @@
>  #ifndef _CEPH_CRYPTO_H
>  #define _CEPH_CRYPTO_H
>  
> +#include <linux/fscrypt.h>
> +
>  #ifdef CONFIG_FS_ENCRYPTION
>  
>  #define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"
> @@ -16,6 +18,29 @@ 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);
>  
> +static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
> +{
> +	if (!IS_ENCRYPTED(parent))
> +		return 0;
> +	return fscrypt_fname_alloc_buffer(NAME_MAX, fname);
> +}
> +
> +static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname)
> +{
> +	if (IS_ENCRYPTED(parent))
> +		fscrypt_fname_free_buffer(fname);
> +}
> +
> +static inline int ceph_get_encryption_info(struct inode *inode)
> +{
> +	if (!IS_ENCRYPTED(inode))
> +		return 0;
> +	return fscrypt_get_encryption_info(inode);
> +}
> +
> +int ceph_fname_to_usr(struct inode *parent, char *name, u32 len,
> +			struct fscrypt_str *tname, struct fscrypt_str *oname);
> +
>  #else /* CONFIG_FS_ENCRYPTION */
>  
>  #define DUMMY_ENCRYPTION_ENABLED(fsc) (0)
> @@ -31,6 +56,28 @@ static inline int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *
>  	return 0;
>  }
>  
> +static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
> +{
> +	return 0;
> +}
> +
> +static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname)
> +{
> +}
> +
> +static inline int ceph_get_encryption_info(struct inode *inode)
> +{
> +	return 0;
> +}

This makes it so that readdir will succeed on encrypted directories when
!CONFIG_FS_ENCRYPTION.  The other filesystems instead return an error code,
which seems much better.  Can you check what the other filesystems handle
readdir?

> +static bool fscrypt_key_status_change(struct dentry *dentry)
> +{
> +	struct inode *dir;
> +	bool encrypted_name, have_key;
> +
> +	lockdep_assert_held(&dentry->d_lock);
> +
> +	dir = d_inode(dentry->d_parent);
> +	if (!IS_ENCRYPTED(dir))
> +		return false;
> +
> +	encrypted_name = dentry->d_flags & DCACHE_ENCRYPTED_NAME;
> +	have_key = fscrypt_has_encryption_key(dir);
> +
> +	if (encrypted_name == have_key)
> +		ceph_dir_clear_complete(dir);
> +
> +	dout("%s encrypted_name=%d have_key=%d\n", __func__, encrypted_name, have_key);
> +	return encrypted_name == have_key;
> +}
> +

Only the no-key => key case needs to be handled, not key => no-key.
Also, the caller already has 'dir', so there's no need to use ->d_parent.

What's wrong with just:

                di = ceph_dentry(dentry);
                if (d_unhashed(dentry) ||
                    d_really_is_negative(dentry) ||
                    di->lease_shared_gen != shared_gen ||
+                   ((dentry->d_flags & DCACHE_ENCRYPTED_NAME) &&
+                    fscrypt_has_encryption_key(dir)))  {
                        spin_unlock(&dentry->d_lock);
                        dput(dentry);
                        err = -EAGAIN;
                        goto out;
                }
>  /*
>   * When possible, we try to satisfy a readdir by peeking at the
>   * dcache.  We make this work by carefully ordering dentries on
> @@ -238,11 +261,11 @@ static int __dcache_readdir(struct file *file,  struct dir_context *ctx,
>  			goto out;
>  		}
>  
> -		spin_lock(&dentry->d_lock);

Why delete this spin_lock()?

> +			if (IS_ENCRYPTED(inode) && !fscrypt_has_encryption_key(inode)) {
> +				spin_lock(&dn->d_lock);
> +				dn->d_flags |= DCACHE_ENCRYPTED_NAME;
> +				spin_unlock(&dn->d_lock);
> +			}

This is racy because fscrypt_has_encryption_key() could have been false when the
dentry was created, then true here.

Take a look at how __fscrypt_prepare_lookup() solves this problem.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets
  2020-09-04 16:05 ` [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
  2020-09-04 16:11   ` Jeff Layton
@ 2020-09-08  5:43   ` Eric Biggers
  1 sibling, 0 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  5:43 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:37PM -0400, Jeff Layton wrote:
> +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> +					   struct delayed_call *done)
> +{
> +	struct ceph_inode_info *ci = ceph_inode(inode);
> +
> +	if (inode->i_link)
> +		return inode->i_link;

Checking ->i_link here is unnecessary since the VFS already does it before
calling inode_operations::get_link().  It's also wrong since it's missing
READ_ONCE() to handle ->i_link being set concurrently.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support
  2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
                   ` (17 preceding siblings ...)
  2020-09-04 16:05 ` [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
@ 2020-09-08  5:54 ` Eric Biggers
  2020-09-08 12:09   ` Jeff Layton
  18 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08  5:54 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Fri, Sep 04, 2020 at 12:05:19PM -0400, Jeff Layton wrote:
> This is a second posting of the ceph+fscrypt integration work that I've
> been experimenting with. The main change with this patch is that I've
> based this on top of Eric's fscrypt-pending set. That necessitated a
> change to allocate inodes much earlier than we have traditionally, prior
> to sending an RPC instead of waiting on the reply.

FWIW, if possible you should create a git tag or branch for your patchset.
While just the mailed patches work fine for *me* for this particular patchset,
other people may not be able to figure out what the patchset applies to.
(In particular, it depends on another patchset:
https://lkml.kernel.org/r/20200824061712.195654-1-ebiggers@kernel.org)

> Note that this just covers the crypto contexts and filenames. I've also
> added a patch to encrypt symlink contents as well, but it doesn't seem to
> be working correctly.

What about symlink encryption isn't working correctly?

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-08  3:38   ` Eric Biggers
@ 2020-09-08 11:27     ` Jeff Layton
  2020-09-08 22:31       ` Eric Biggers
  0 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 11:27 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 20:38 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:20PM -0400, Jeff Layton wrote:
> > Ceph needs to be able to allocate inodes ahead of a create that might
> > involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
> > but it puts the inode on the sb->s_inodes list, and we don't want to
> > do that until we're ready to insert it into the hash.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/inode.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 72c4c347afb7..61554c2477ab 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
> >  	}
> >  	return inode;
> >  }
> > +EXPORT_SYMBOL(new_inode_pseudo);
> >  
> 
> What's the problem with putting the new inode on sb->s_inodes already?
> That's what all the other filesystems do.
> 

The existing ones are all local filesystems that use
insert_inode_locked() and similar paths. Ceph needs to use the '5'
variants of those functions (inode_insert5(), iget5_locked(), etc.).

When we go to insert it into the hash in inode_insert5(), we'd need to
set I_CREATING if allocated from new_inode(). But, if you do _that_,
then you'll get back ESTALE from find_inode() if (eg.) someone calls
iget5_locked() before you can clear I_CREATING.

Hitting that race is easy with an asynchronous create. The simplest
scheme to avoid that is to just export new_inode_pseudo and keep it off
of s_inodes until we're ready to do the insert. The only real issue here
is that this inode won't be findable by evict_inodes during umount, but
that shouldn't be happening during an active syscall anyway.
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode
  2020-09-08  3:48   ` Eric Biggers
@ 2020-09-08 11:29     ` Jeff Layton
  2020-09-08 12:29     ` Jeff Layton
  1 sibling, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 11:29 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 20:48 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:23PM -0400, Jeff Layton wrote:
> > CephFS will need to be able to generate a context for a new "prepared"
> > inode. Add a new routine for getting the context out of an in-core
> > inode.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/policy.c      | 20 ++++++++++++++++++++
> >  include/linux/fscrypt.h |  1 +
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> > index c56ad886f7d7..10eddd113a21 100644
> > --- a/fs/crypto/policy.c
> > +++ b/fs/crypto/policy.c
> > @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
> >  }
> >  EXPORT_SYMBOL_GPL(fscrypt_set_context);
> >  
> > +/**
> > + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode
> 
> Comment doesn't match the function name.
> 
> Also, the name isn't very clear.  How about calling this
> fscrypt_context_for_new_inode()?
> 
> BTW, I might rename fscrypt_new_context_from_policy() to
> fscrypt_context_from_policy() in my patchset.  Since it now makes the caller
> provide the nonce, technically it's no longer limited to "new" contexts.
> 

Ahh yes. I didn't properly update the commit message here. Your
suggested names sound fine. I'll plan to fix that up.

> > + * @ctx: where context should be written
> > + * @inode: inode from which to fetch context
> > + *
> > + * Given an in-core prepared, but not-necessarily fully-instantiated inode,
> > + * generate an encryption context from its policy and write it to ctx.
> 
> Clarify what is meant by "prepared" (fscrypt_prepare_new_inode() was called)
> vs. "instantiated".
> 

Ack.

> > + *
> > + * Returns size of the context.
> > + */
> > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode)
> > +{
> > +	struct fscrypt_info *ci = inode->i_crypt_info;
> > +
> > +	BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
> > +
> > +	return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce);
> > +}
> > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode);
> 
> fscrypt_set_context() should be changed to call this, instead of duplicating the
> same logic.  As part of that, the WARN_ON_ONCE(!ci) that's currently in
> fscrypt_set_context() should go in here instead.

Ok.
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support
  2020-09-08  5:54 ` [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Eric Biggers
@ 2020-09-08 12:09   ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 12:09 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 22:54 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:19PM -0400, Jeff Layton wrote:
> > This is a second posting of the ceph+fscrypt integration work that I've
> > been experimenting with. The main change with this patch is that I've
> > based this on top of Eric's fscrypt-pending set. That necessitated a
> > change to allocate inodes much earlier than we have traditionally, prior
> > to sending an RPC instead of waiting on the reply.
> 
> FWIW, if possible you should create a git tag or branch for your patchset.
> While just the mailed patches work fine for *me* for this particular patchset,
> other people may not be able to figure out what the patchset applies to.
> (In particular, it depends on another patchset:
> https://lkml.kernel.org/r/20200824061712.195654-1-ebiggers@kernel.org)
> 

I've tagged this out as 'ceph-fscrypt-rfc.2' in my kernel.org tree (the
first posting is ceph-fscrypt-rfc.1).

Note that this also is layered on top of David Howell's fscache rework,
and the work I've done to adapt cephfs to that.

> > Note that this just covers the crypto contexts and filenames. I've also
> > added a patch to encrypt symlink contents as well, but it doesn't seem to
> > be working correctly.
> 
> What about symlink encryption isn't working correctly?

What I was seeing is that after unmounting and mounting, the symlink
contents would be gibberish when read by readlink(). I confirmed that
the same crypttext that came out of fscrypt_encrypt_symlink() was being
fed into fscrypt_get_symlink(), but the result from that came back as
gibberish.

I need to do a bit more troubleshooting, but I now wonder if it's due to
the context handling being wrong when dummy encryption is enabled. I'll
have a look at that soon.

Thanks for the review so far!
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode
  2020-09-08  3:48   ` Eric Biggers
  2020-09-08 11:29     ` Jeff Layton
@ 2020-09-08 12:29     ` Jeff Layton
  2020-09-08 22:34       ` Eric Biggers
  1 sibling, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 12:29 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 20:48 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:23PM -0400, Jeff Layton wrote:
> > CephFS will need to be able to generate a context for a new "prepared"
> > inode. Add a new routine for getting the context out of an in-core
> > inode.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/policy.c      | 20 ++++++++++++++++++++
> >  include/linux/fscrypt.h |  1 +
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> > index c56ad886f7d7..10eddd113a21 100644
> > --- a/fs/crypto/policy.c
> > +++ b/fs/crypto/policy.c
> > @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
> >  }
> >  EXPORT_SYMBOL_GPL(fscrypt_set_context);
> >  
> > +/**
> > + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode
> 
> Comment doesn't match the function name.
> 
> Also, the name isn't very clear.  How about calling this
> fscrypt_context_for_new_inode()?
> 
> BTW, I might rename fscrypt_new_context_from_policy() to
> fscrypt_context_from_policy() in my patchset.  Since it now makes the caller
> provide the nonce, technically it's no longer limited to "new" contexts.
> 
> > + * @ctx: where context should be written
> > + * @inode: inode from which to fetch context
> > + *
> > + * Given an in-core prepared, but not-necessarily fully-instantiated inode,
> > + * generate an encryption context from its policy and write it to ctx.
> 
> Clarify what is meant by "prepared" (fscrypt_prepare_new_inode() was called)
> vs. "instantiated".
> 
> > + *
> > + * Returns size of the context.
> > + */
> > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode)
> > +{
> > +	struct fscrypt_info *ci = inode->i_crypt_info;
> > +
> > +	BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
> > +
> > +	return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce);
> > +}
> > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode);
> 
> fscrypt_set_context() should be changed to call this, instead of duplicating the
> same logic.  As part of that, the WARN_ON_ONCE(!ci) that's currently in
> fscrypt_set_context() should go in here instead.
> 

Note that we can't just move that WARN_ON_ONCE. If we do that, then
fscrypt_set_context will dereference ci before that check can occur, so
we'd be trading a warning and -ENOKEY for a NULL pointer dereference. I
think we'll have to duplicate that in both functions.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it
  2020-09-08  3:55   ` Eric Biggers
@ 2020-09-08 12:50     ` Jeff Layton
  2020-09-08 22:53       ` Eric Biggers
  0 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 12:50 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 20:55 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:25PM -0400, Jeff Layton wrote:
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/fname.c       | 71 +++++++++++++++++++++++------------------
> >  include/linux/fscrypt.h |  3 ++
> >  2 files changed, 43 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> > index 9440a44e24ac..09f09def87fc 100644
> > --- a/fs/crypto/fname.c
> > +++ b/fs/crypto/fname.c
> > @@ -300,6 +300,45 @@ void fscrypt_fname_free_buffer(struct fscrypt_str *crypto_str)
> >  }
> >  EXPORT_SYMBOL(fscrypt_fname_free_buffer);
> >  
> > +void fscrypt_encode_nokey_name(u32 hash, u32 minor_hash,
> > +			     const struct fscrypt_str *iname,
> > +			     struct fscrypt_str *oname)
> > +{
> > +	struct fscrypt_nokey_name nokey_name;
> > +	u32 size; /* size of the unencoded no-key name */
> > +
> > +	/*
> > +	 * Sanity check that struct fscrypt_nokey_name doesn't have padding
> > +	 * between fields and that its encoded size never exceeds NAME_MAX.
> > +	 */
> > +	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, dirhash) !=
> > +		     offsetof(struct fscrypt_nokey_name, bytes));
> > +	BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, bytes) !=
> > +		     offsetof(struct fscrypt_nokey_name, sha256));
> > +	BUILD_BUG_ON(BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX) > NAME_MAX);
> > +
> > +	if (hash) {
> > +		nokey_name.dirhash[0] = hash;
> > +		nokey_name.dirhash[1] = minor_hash;
> > +	} else {
> > +		nokey_name.dirhash[0] = 0;
> > +		nokey_name.dirhash[1] = 0;
> > +	}
> > +	if (iname->len <= sizeof(nokey_name.bytes)) {
> > +		memcpy(nokey_name.bytes, iname->name, iname->len);
> > +		size = offsetof(struct fscrypt_nokey_name, bytes[iname->len]);
> > +	} else {
> > +		memcpy(nokey_name.bytes, iname->name, sizeof(nokey_name.bytes));
> > +		/* Compute strong hash of remaining part of name. */
> > +		fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)],
> > +				  iname->len - sizeof(nokey_name.bytes),
> > +				  nokey_name.sha256);
> > +		size = FSCRYPT_NOKEY_NAME_MAX;
> > +	}
> > +	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
> > +}
> > +EXPORT_SYMBOL(fscrypt_encode_nokey_name);
> 
> Why does this need to be exported?
> 
> There's no user of this function introduced in this patchset.
> 
> - Eric

Yeah, I probably should have dropped this from the series for now as
nothing uses it yet, but eventually we may need this. I did a fairly
detailed writeup of the problem here:

    https://tracker.ceph.com/issues/47162

Basically, we still need to allow clients to look up dentries in the MDS
even when they don't have the key.

There are a couple of different approaches, but the simplest is to just
have the client always store long dentry names using the nokey_name, and
then keep the full name in a new field in the dentry representation that
is sent across the wire.

This requires some changes to the Ceph MDS (which is what that tracker
bug is about), and will mean enshrining the nokey name in perpetuity.
We're still looking at this for now though, and we're open to other
approaches if you've got any to suggest.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/
  2020-09-08  3:59   ` Eric Biggers
@ 2020-09-08 12:51     ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 12:51 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 20:59 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:26PM -0400, Jeff Layton wrote:
> > Once we allow encrypted filenames on ceph we'll end up with names that
> > may have illegal characters in them (embedded '\0' or '/'), or
> > characters that aren't printable.
> > 
> > It will be safer to use strings that are printable. It turns out that the
> > MDS doesn't really care about the length of filenames, so we can just
> > base64 encode and decode filenames before writing and reading them.
> > 
> > Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
> > select it when it's enabled.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/Kconfig            |  1 +
> >  fs/crypto/fname.c            | 64 ++------------------------------
> >  include/linux/base64_fname.h | 11 ++++++
> >  lib/Kconfig                  |  3 ++
> >  lib/Makefile                 |  1 +
> >  lib/base64_fname.c           | 71 ++++++++++++++++++++++++++++++++++++
> >  6 files changed, 90 insertions(+), 61 deletions(-)
> >  create mode 100644 include/linux/base64_fname.h
> >  create mode 100644 lib/base64_fname.c
> > 
> 
> I'm still concerned that this functionality is too specific to belong in lib/ at
> the moment, given that it's not the most commonly used variant of base64.  How
> about keeping these functions in fs/crypto/ for now?  You can call them
> fscrypt_base64_encode() and fscrypt_base64_decode() and export them for ceph to
> use.
> 

Ok, will do.

> > diff --git a/lib/base64_fname.c b/lib/base64_fname.c
> > new file mode 100644
> > index 000000000000..7638c45e4035
> > --- /dev/null
> > +++ b/lib/base64_fname.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Modified base64 encode/decode functions, suitable for use as filename components.
> > + *
> > + * Originally lifted from fs/crypto/fname.c
> > + *
> > + * Copyright (C) 2015, Jaegeuk Kim
> > + * Copyright (C) 2015, Eric Biggers
> > + */
> 
> Please don't change the copyright statements.  The original file had:
> 
>  * Copyright (C) 2015, Google, Inc.
>  * Copyright (C) 2015, Motorola Mobility
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted
  2020-09-08  3:52   ` Eric Biggers
@ 2020-09-08 12:54     ` Jeff Layton
  2020-09-08 23:08       ` Eric Biggers
  0 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 12:54 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 20:52 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:24PM -0400, Jeff Layton wrote:
> > Cephfs (currently) sets this flag early and only fetches the context
> > later. Eventually we may not need this, but for now it prevents this
> > warning from popping.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/keysetup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> > index ad64525ec680..3b4ec16fc528 100644
> > --- a/fs/crypto/keysetup.c
> > +++ b/fs/crypto/keysetup.c
> > @@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
> >  		const union fscrypt_context *dummy_ctx =
> >  			fscrypt_get_dummy_context(inode->i_sb);
> >  
> > -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> > +		if (!dummy_ctx) {
> >  			fscrypt_warn(inode,
> >  				     "Error %d getting encryption context",
> >  				     res);
> 
> This makes errors reading the encryption xattr of an encrypted inode be ignored
> when the filesystem is mounted with test_dummy_encryption.  That's undesirable.
> 
> Isn't this change actually no longer needed, now that new inodes will use
> fscrypt_prepare_new_inode() instead of fscrypt_get_encryption_info()?

No. This is really for when we're reading in a new inode from the MDS.
We can tell that there is a context present in some of those cases, but
may not be able to read it yet. That said, it may be possible to pull in
the context at the point where we set S_ENCRYPTED. I'll take a look.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 09/18] ceph: crypto context handling for ceph
  2020-09-08  4:29   ` Eric Biggers
@ 2020-09-08 16:14     ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-08 16:14 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 21:29 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:28PM -0400, Jeff Layton wrote:
> > Store the fscrypt context for an inode as an encryption.ctx xattr.
> > 
> > Also add support for "dummy" encryption (useful for testing with
> > automated test harnesses like xfstests).
> 
> Can you put the test_dummy_encryption support in a separate patch?
> 
> > +static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > +{
> > +	int ret = __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);
> > +
> > +	if (ret > 0)
> > +		inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
> > +	return ret;
> > +}
> > +
> > +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;
> > +}
> 
> get_context() shouldn't be setting the S_ENCRYPTED inode flag.
> Only set_context() should be doing that.
> 
> > +
> > +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 const union fscrypt_context *
> > +ceph_get_dummy_context(struct super_block *sb)
> > +{
> > +	return ceph_sb_to_client(sb)->dummy_enc_ctx.ctx;
> > +}
> > +
> > +static struct fscrypt_operations ceph_fscrypt_ops = {
> > +	.key_prefix		= "ceph:",
> 
> IMO you shouldn't set .key_prefix here, since it's deprecated.
> Just leave it unset so that ceph will only support the generic prefix "fscrypt:"
> as well as FS_IOC_ADD_ENCRYPTION_KEY.
> 
> >  enum ceph_recover_session_mode {
> > @@ -197,6 +200,8 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
> >  	fsparam_u32	("rsize",			Opt_rsize),
> >  	fsparam_string	("snapdirname",			Opt_snapdirname),
> >  	fsparam_string	("source",			Opt_source),
> > +	fsparam_flag_no ("test_dummy_encryption",	Opt_test_dummy_encryption),
> > +	fsparam_string	("test_dummy_encryption",	Opt_test_dummy_encryption),
> 
> I think you should use fsparam_flag instead of fsparam_flag_no, since otherwise
> "notest_dummy_encryption" will be recognized.  There's not a problem with it
> per se, but none of the other filesystems that support "test_dummy_encryption"
> allow "notest_dummy_encryption".  It's nice to keep things consistent.
> 
> I.e. if "notest_dummy_encryption" is really something that would be useful
> (presumably only for remount, since it's a test-only option that will never be
> on by default), then we should add it to ext4, f2fs, and ceph -- not just ceph.
> 
> > +	/* Don't allow test_dummy_encryption to change on remount */
> > +	if (fsopt->flags & CEPH_MOUNT_OPT_TEST_DUMMY_ENC) {
> > +		if (!ceph_test_mount_opt(fsc, TEST_DUMMY_ENC))
> > +			return -EEXIST;
> > +	} else {
> > +		if (ceph_test_mount_opt(fsc, TEST_DUMMY_ENC))
> > +			return -EEXIST;
> > +	}
> > +
> 
> Can you check what ext4 and f2fs do for this?  test_dummy_encryption isn't just
> a boolean flag anymore, so this logic isn't sufficient to prevent it from
> changing during remount.  For example someone could mount with
> test_dummy_encryption=v1, then try to remount with test_dummy_encryption=v2.
> On ext4 and f2fs, that intentionally fails.

Ok, I'll see what I can do. Note that those fs' all use the old mount
API (so far) and ceph has been converted to the new one. We may need to
rework a bit of the fscrypt infrastructure to handle the new mount API.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-08 11:27     ` Jeff Layton
@ 2020-09-08 22:31       ` Eric Biggers
  2020-09-09 10:47         ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08 22:31 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Tue, Sep 08, 2020 at 07:27:58AM -0400, Jeff Layton wrote:
> On Mon, 2020-09-07 at 20:38 -0700, Eric Biggers wrote:
> > On Fri, Sep 04, 2020 at 12:05:20PM -0400, Jeff Layton wrote:
> > > Ceph needs to be able to allocate inodes ahead of a create that might
> > > involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
> > > but it puts the inode on the sb->s_inodes list, and we don't want to
> > > do that until we're ready to insert it into the hash.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/inode.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/fs/inode.c b/fs/inode.c
> > > index 72c4c347afb7..61554c2477ab 100644
> > > --- a/fs/inode.c
> > > +++ b/fs/inode.c
> > > @@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
> > >  	}
> > >  	return inode;
> > >  }
> > > +EXPORT_SYMBOL(new_inode_pseudo);
> > >  
> > 
> > What's the problem with putting the new inode on sb->s_inodes already?
> > That's what all the other filesystems do.
> > 
> 
> The existing ones are all local filesystems that use
> insert_inode_locked() and similar paths. Ceph needs to use the '5'
> variants of those functions (inode_insert5(), iget5_locked(), etc.).
> 
> When we go to insert it into the hash in inode_insert5(), we'd need to
> set I_CREATING if allocated from new_inode(). But, if you do _that_,
> then you'll get back ESTALE from find_inode() if (eg.) someone calls
> iget5_locked() before you can clear I_CREATING.
> 
> Hitting that race is easy with an asynchronous create. The simplest
> scheme to avoid that is to just export new_inode_pseudo and keep it off
> of s_inodes until we're ready to do the insert. The only real issue here
> is that this inode won't be findable by evict_inodes during umount, but
> that shouldn't be happening during an active syscall anyway.

Is your concern the following scenario?

1. ceph successfully created a new file on the server
2. inode_insert5() is called for the new file's inode
3. error occurs in ceph_fill_inode()
4. discard_new_inode() is called
5. another thread looks up the inode and gets ESTALE
6. iput() is finally called

And the claim is that the ESTALE in (5) is unexpected?  I'm not sure that it's
unexpected, given that the file allegedly failed to be created...  Also, it
seems that maybe (3) isn't something that should actually happen, since after
(1) it's too late to fail the file creation.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode
  2020-09-08 12:29     ` Jeff Layton
@ 2020-09-08 22:34       ` Eric Biggers
  0 siblings, 0 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-08 22:34 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Tue, Sep 08, 2020 at 08:29:52AM -0400, Jeff Layton wrote:
> > > + *
> > > + * Returns size of the context.
> > > + */
> > > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode)
> > > +{
> > > +	struct fscrypt_info *ci = inode->i_crypt_info;
> > > +
> > > +	BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
> > > +
> > > +	return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce);
> > > +}
> > > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode);
> > 
> > fscrypt_set_context() should be changed to call this, instead of duplicating the
> > same logic.  As part of that, the WARN_ON_ONCE(!ci) that's currently in
> > fscrypt_set_context() should go in here instead.
> > 
> 
> Note that we can't just move that WARN_ON_ONCE. If we do that, then
> fscrypt_set_context will dereference ci before that check can occur, so
> we'd be trading a warning and -ENOKEY for a NULL pointer dereference. I
> think we'll have to duplicate that in both functions.

You could just make fscrypt_set_context() call fscrypt_new_context_from_inode()
first, before the fscrypt_hash_inode_number() stuff.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it
  2020-09-08 12:50     ` Jeff Layton
@ 2020-09-08 22:53       ` Eric Biggers
  2020-09-09 16:02         ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-08 22:53 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Tue, Sep 08, 2020 at 08:50:04AM -0400, Jeff Layton wrote:
> > > +EXPORT_SYMBOL(fscrypt_encode_nokey_name);
> > 
> > Why does this need to be exported?
> > 
> > There's no user of this function introduced in this patchset.
> > 
> > - Eric
> 
> Yeah, I probably should have dropped this from the series for now as
> nothing uses it yet, but eventually we may need this. I did a fairly
> detailed writeup of the problem here:
> 
>     https://tracker.ceph.com/issues/47162
> 
> Basically, we still need to allow clients to look up dentries in the MDS
> even when they don't have the key.
> 
> There are a couple of different approaches, but the simplest is to just
> have the client always store long dentry names using the nokey_name, and
> then keep the full name in a new field in the dentry representation that
> is sent across the wire.
> 
> This requires some changes to the Ceph MDS (which is what that tracker
> bug is about), and will mean enshrining the nokey name in perpetuity.
> We're still looking at this for now though, and we're open to other
> approaches if you've got any to suggest.

The (persistent) directory entries have to include the full ciphertext
filenames.  If they only included the no-key names, then it wouldn't always be
possible to translate them back into the original plaintext filenames.

It's also required that the filesystem can find a specific directory entry given
its corresponding no-key name.  For a network filesystem, that can be done
either on the client (request all filenames in the directory, then check all of
them...), or on the server (give it the no-key name and have it do the matching;
it would need to know the specifics of how the no-key names work).

The no-key names aren't currently stable, and it would be nice to keep them that
way since we might want to improve how they work later.  But if you need to
stabilize a version of the no-key name format for use in the ceph protocol so
that the server can do the matching, it would be possible to do that.  It
wouldn't even necessarily have to be what fscrypt currently uses; e.g. if it
were to simplify things a lot for you to simply use SHA-256(ciphertext_name)
instead of the current 'struct fscrypt_nokey_name', you could do that.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted
  2020-09-08 12:54     ` Jeff Layton
@ 2020-09-08 23:08       ` Eric Biggers
  0 siblings, 0 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-08 23:08 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Tue, Sep 08, 2020 at 08:54:35AM -0400, Jeff Layton wrote:
> On Mon, 2020-09-07 at 20:52 -0700, Eric Biggers wrote:
> > On Fri, Sep 04, 2020 at 12:05:24PM -0400, Jeff Layton wrote:
> > > Cephfs (currently) sets this flag early and only fetches the context
> > > later. Eventually we may not need this, but for now it prevents this
> > > warning from popping.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/crypto/keysetup.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> > > index ad64525ec680..3b4ec16fc528 100644
> > > --- a/fs/crypto/keysetup.c
> > > +++ b/fs/crypto/keysetup.c
> > > @@ -567,7 +567,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
> > >  		const union fscrypt_context *dummy_ctx =
> > >  			fscrypt_get_dummy_context(inode->i_sb);
> > >  
> > > -		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
> > > +		if (!dummy_ctx) {
> > >  			fscrypt_warn(inode,
> > >  				     "Error %d getting encryption context",
> > >  				     res);
> > 
> > This makes errors reading the encryption xattr of an encrypted inode be ignored
> > when the filesystem is mounted with test_dummy_encryption.  That's undesirable.
> > 
> > Isn't this change actually no longer needed, now that new inodes will use
> > fscrypt_prepare_new_inode() instead of fscrypt_get_encryption_info()?
> 
> No. This is really for when we're reading in a new inode from the MDS.
> We can tell that there is a context present in some of those cases, but
> may not be able to read it yet. That said, it may be possible to pull in
> the context at the point where we set S_ENCRYPTED. I'll take a look.

fscrypt_prepare_new_inode() sets ->i_crypt_info on the new inode.
fscrypt_get_encryption_info() isn't needed after that.  If it does get called
anyway, it will be a no-op since it will see fscrypt_has_encryption_key(inode).

And when allocating an in-memory inode for an existing file,
fscrypt_get_encryption_info() shouldn't be called either.  It gets called later
by filesystem operations that need the encryption key, usually ->open().

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-08 22:31       ` Eric Biggers
@ 2020-09-09 10:47         ` Jeff Layton
  2020-09-09 16:12           ` Eric Biggers
  0 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 10:47 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Tue, 2020-09-08 at 15:31 -0700, Eric Biggers wrote:
> On Tue, Sep 08, 2020 at 07:27:58AM -0400, Jeff Layton wrote:
> > On Mon, 2020-09-07 at 20:38 -0700, Eric Biggers wrote:
> > > On Fri, Sep 04, 2020 at 12:05:20PM -0400, Jeff Layton wrote:
> > > > Ceph needs to be able to allocate inodes ahead of a create that might
> > > > involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
> > > > but it puts the inode on the sb->s_inodes list, and we don't want to
> > > > do that until we're ready to insert it into the hash.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/inode.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/fs/inode.c b/fs/inode.c
> > > > index 72c4c347afb7..61554c2477ab 100644
> > > > --- a/fs/inode.c
> > > > +++ b/fs/inode.c
> > > > @@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
> > > >  	}
> > > >  	return inode;
> > > >  }
> > > > +EXPORT_SYMBOL(new_inode_pseudo);
> > > >  
> > > 
> > > What's the problem with putting the new inode on sb->s_inodes already?
> > > That's what all the other filesystems do.
> > > 
> > 
> > The existing ones are all local filesystems that use
> > insert_inode_locked() and similar paths. Ceph needs to use the '5'
> > variants of those functions (inode_insert5(), iget5_locked(), etc.).
> > 
> > When we go to insert it into the hash in inode_insert5(), we'd need to
> > set I_CREATING if allocated from new_inode(). But, if you do _that_,
> > then you'll get back ESTALE from find_inode() if (eg.) someone calls
> > iget5_locked() before you can clear I_CREATING.
> > 
> > Hitting that race is easy with an asynchronous create. The simplest
> > scheme to avoid that is to just export new_inode_pseudo and keep it off
> > of s_inodes until we're ready to do the insert. The only real issue here
> > is that this inode won't be findable by evict_inodes during umount, but
> > that shouldn't be happening during an active syscall anyway.
> 
> Is your concern the following scenario?
> 
> 1. ceph successfully created a new file on the server
> 2. inode_insert5() is called for the new file's inode
> 3. error occurs in ceph_fill_inode()
> 4. discard_new_inode() is called
> 5. another thread looks up the inode and gets ESTALE
> 6. iput() is finally called
> 
> And the claim is that the ESTALE in (5) is unexpected?  I'm not sure that it's
> unexpected, given that the file allegedly failed to be created...  Also, it
> seems that maybe (3) isn't something that should actually happen, since after
> (1) it's too late to fail the file creation.
> 

No, more like:

Syscall					Workqueue
------------------------------------------------------------------------------
1. allocate an inode
2. determine we can do an async create
   and allocate an inode number for it
3. hash the inode (must set I_CREATING
   if we allocated with new_inode()) 
4. issue the call to the MDS
5. finish filling out the inode()
6.					MDS reply comes in, and workqueue thread
					looks up new inode (-ESTALE)
7. unlock_new_inode()


Because 6 happens before 7 in this case, we get an ESTALE on that
lookup. By using new_inode_pseudo() and not setting I_CREATING, 6 ends
up waiting on the inode to be unlocked rather than giving up.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  2020-09-08  4:57   ` Eric Biggers
@ 2020-09-09 12:20     ` Jeff Layton
  2020-09-09 15:53     ` Jeff Layton
  1 sibling, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 12:20 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

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@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path
  2020-09-08  5:06   ` Eric Biggers
@ 2020-09-09 12:24     ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 12:24 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt, Xiubo Li

On Mon, 2020-09-07 at 22:06 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:33PM -0400, Jeff Layton wrote:
> > Allow ceph_mdsc_build_path to encrypt and base64 encode the filename
> > when the parent is encrypted and we're sending the path to the MDS.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/mds_client.c | 51 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 40 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index e3dc061252d4..26b43ae09823 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/ratelimit.h>
> >  #include <linux/bits.h>
> >  #include <linux/ktime.h>
> > +#include <linux/base64_fname.h>
> >  
> >  #include "super.h"
> >  #include "mds_client.h"
> > @@ -2324,8 +2325,7 @@ static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
> >   * Encode hidden .snap dirs as a double /, i.e.
> >   *   foo/.snap/bar -> foo//bar
> >   */
> > -char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> > -			   int stop_on_nosnap)
> > +char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire)
> >  {
> >  	struct dentry *cur;
> >  	struct inode *inode;
> > @@ -2347,30 +2347,59 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> >  	seq = read_seqbegin(&rename_lock);
> >  	cur = dget(dentry);
> >  	for (;;) {
> > -		struct dentry *temp;
> > +		struct dentry *parent;
> >  
> >  		spin_lock(&cur->d_lock);
> >  		inode = d_inode(cur);
> > +		parent = cur->d_parent;
> >  		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
> >  			dout("build_path path+%d: %p SNAPDIR\n",
> >  			     pos, cur);
> > -		} else if (stop_on_nosnap && inode && dentry != cur &&
> > -			   ceph_snap(inode) == CEPH_NOSNAP) {
> > +			dget(parent);
> > +			spin_unlock(&cur->d_lock);
> > +		} else if (for_wire && inode && dentry != cur && ceph_snap(inode) == CEPH_NOSNAP) {
> >  			spin_unlock(&cur->d_lock);
> >  			pos++; /* get rid of any prepended '/' */
> >  			break;
> > -		} else {
> > +		} else if (!for_wire || !IS_ENCRYPTED(d_inode(parent))) {
> >  			pos -= cur->d_name.len;
> >  			if (pos < 0) {
> >  				spin_unlock(&cur->d_lock);
> >  				break;
> >  			}
> >  			memcpy(path + pos, cur->d_name.name, cur->d_name.len);
> > +			dget(parent);
> > +			spin_unlock(&cur->d_lock);
> > +		} else {
> > +			int err;
> > +			struct fscrypt_name fname = { };
> > +			int len;
> > +			char buf[BASE64_CHARS(NAME_MAX)];
> > +
> > +			dget(parent);
> > +			spin_unlock(&cur->d_lock);
> > +
> > +			err = fscrypt_setup_filename(d_inode(parent), &cur->d_name, 1, &fname);
> 
> How are no-key filenames handled with ceph?  You're calling
> fscrypt_setup_filename() with lookup=1, so it will give you back a no-key
> filename if the directory's encryption key is unavailable.
> 

Still TBD.

For now, I'm just ignoring long filenames. Eventually we'll need to
extend the MDS and protocol to handle the nokey names properly and this
code will need to be reworked.

I have this bug opened for tracking that work:

    https://tracker.ceph.com/issues/47162

 
> > +			if (err) {
> > +				dput(parent);
> > +				dput(cur);
> > +				return ERR_PTR(err);
> > +			}
> > +
> > +			/* base64 encode the encrypted name */
> > +			len = base64_encode_fname(fname.disk_name.name, fname.disk_name.len, buf);
> > +			pos -= len;
> > +			if (pos < 0) {
> > +				dput(parent);
> > +				fscrypt_free_filename(&fname);
> > +				break;
> > +			}
> > +			memcpy(path + pos, buf, len);
> > +			dout("non-ciphertext name = %.*s\n", len, buf);
> > +			fscrypt_free_filename(&fname);
> 
> This would be easier to understand if the encryption and encoding logic was
> moved into its own function.
> 


Agreed, though it's a little hard given the way this function is
structured. I'll see what I can do to clean it up though.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries
  2020-09-08  5:12   ` Eric Biggers
@ 2020-09-09 12:26     ` Jeff Layton
  2020-09-09 16:18       ` Eric Biggers
  0 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 12:26 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 22:12 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:34PM -0400, Jeff Layton wrote:
> > If we have an encrypted dentry, then we need to test whether a new key
> > might have been established or removed. Do that before we test anything
> > else about the dentry.
> 
> A more accurate explanation would be:
> 
> "If we have a dentry which represents a no-key name, then we need to test
>  whether the parent directory's encryption key has since been added."
> 

Can't a key also be removed (e.g. fscrypt lock /path/to/dir)?

Does that result in the dentries below that point being invalidated?

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/dir.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index b3f2741becdb..cc85933413b9 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -1695,6 +1695,12 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
> >  	dout("d_revalidate %p '%pd' inode %p offset 0x%llx\n", dentry,
> >  	     dentry, inode, ceph_dentry(dentry)->offset);
> >  
> > +	if (IS_ENCRYPTED(dir)) {
> > +		valid = fscrypt_d_revalidate(dentry, flags);
> > +		if (valid <= 0)
> > +			return valid;
> > +	}
> 
> There's no need to check IS_ENCRYPTED(dir) here.
> 

Thanks, fixed in my tree.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames
  2020-09-08  5:34   ` Eric Biggers
@ 2020-09-09 13:02     ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 13:02 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Mon, 2020-09-07 at 22:34 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:35PM -0400, Jeff Layton wrote:
> > diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> > index 1b11e9af165e..88c672ccdcf8 100644
> > --- a/fs/ceph/crypto.h
> > +++ b/fs/ceph/crypto.h
> > @@ -6,6 +6,8 @@
> >  #ifndef _CEPH_CRYPTO_H
> >  #define _CEPH_CRYPTO_H
> >  
> > +#include <linux/fscrypt.h>
> > +
> >  #ifdef CONFIG_FS_ENCRYPTION
> >  
> >  #define	CEPH_XATTR_NAME_ENCRYPTION_CONTEXT	"encryption.ctx"
> > @@ -16,6 +18,29 @@ 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);
> >  
> > +static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
> > +{
> > +	if (!IS_ENCRYPTED(parent))
> > +		return 0;
> > +	return fscrypt_fname_alloc_buffer(NAME_MAX, fname);
> > +}
> > +
> > +static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname)
> > +{
> > +	if (IS_ENCRYPTED(parent))
> > +		fscrypt_fname_free_buffer(fname);
> > +}
> > +
> > +static inline int ceph_get_encryption_info(struct inode *inode)
> > +{
> > +	if (!IS_ENCRYPTED(inode))
> > +		return 0;
> > +	return fscrypt_get_encryption_info(inode);
> > +}
> > +
> > +int ceph_fname_to_usr(struct inode *parent, char *name, u32 len,
> > +			struct fscrypt_str *tname, struct fscrypt_str *oname);
> > +
> >  #else /* CONFIG_FS_ENCRYPTION */
> >  
> >  #define DUMMY_ENCRYPTION_ENABLED(fsc) (0)
> > @@ -31,6 +56,28 @@ static inline int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *
> >  	return 0;
> >  }
> >  
> > +static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname)
> > +{
> > +}
> > +
> > +static inline int ceph_get_encryption_info(struct inode *inode)
> > +{
> > +	return 0;
> > +}
> 
> This makes it so that readdir will succeed on encrypted directories when
> !CONFIG_FS_ENCRYPTION.  The other filesystems instead return an error code,
> which seems much better.  Can you check what the other filesystems handle
> readdir?
>

Maybe. I'm not sure it's better.

It would be nice to be able to allow such clients to be able to clean
out an encrypted tree (given appropriate permissions, of course).

A network filesystem like this is a much different case than a local
one. We may have a swath of varying client kernel versions and
configurations that are operating on the same filesystem.

Where we draw that line is still being determined though.

> > +static bool fscrypt_key_status_change(struct dentry *dentry)
> > +{
> > +	struct inode *dir;
> > +	bool encrypted_name, have_key;
> > +
> > +	lockdep_assert_held(&dentry->d_lock);
> > +
> > +	dir = d_inode(dentry->d_parent);
> > +	if (!IS_ENCRYPTED(dir))
> > +		return false;
> > +
> > +	encrypted_name = dentry->d_flags & DCACHE_ENCRYPTED_NAME;
> > +	have_key = fscrypt_has_encryption_key(dir);
> > +
> > +	if (encrypted_name == have_key)
> > +		ceph_dir_clear_complete(dir);
> > +
> > +	dout("%s encrypted_name=%d have_key=%d\n", __func__, encrypted_name, have_key);
> > +	return encrypted_name == have_key;
> > +}
> > +
> 
> Only the no-key => key case needs to be handled, not key => no-key.
> Also, the caller already has 'dir', so there's no need to use ->d_parent.
> 
> What's wrong with just:
> 
>                 di = ceph_dentry(dentry);
>                 if (d_unhashed(dentry) ||
>                     d_really_is_negative(dentry) ||
>                     di->lease_shared_gen != shared_gen ||
> +                   ((dentry->d_flags & DCACHE_ENCRYPTED_NAME) &&
> +                    fscrypt_has_encryption_key(dir)))  {
>                         spin_unlock(&dentry->d_lock);
>                         dput(dentry);
>                         err = -EAGAIN;
>                         goto out;
>                 }

Ok, I didn't realize that I didn't need to worry about key removal. Your
proposed scheme is simpler.

> >  /*
> >   * When possible, we try to satisfy a readdir by peeking at the
> >   * dcache.  We make this work by carefully ordering dentries on
> > @@ -238,11 +261,11 @@ static int __dcache_readdir(struct file *file,  struct dir_context *ctx,
> >  			goto out;
> >  		}
> >  
> > -		spin_lock(&dentry->d_lock);
> 
> Why delete this spin_lock()?
> 

Yikes -- good catch! Fixed.

> > +			if (IS_ENCRYPTED(inode) && !fscrypt_has_encryption_key(inode)) {
> > +				spin_lock(&dn->d_lock);
> > +				dn->d_flags |= DCACHE_ENCRYPTED_NAME;
> > +				spin_unlock(&dn->d_lock);
> > +			}
> 
> This is racy because fscrypt_has_encryption_key() could have been false when the
> dentry was created, then true here.
> 
> Take a look at how __fscrypt_prepare_lookup() solves this problem.

Blech.

I guess I need to have ceph_fname_to_usr tell whether the name is a
nokey name, but that info is not currently returned
by fscrypt_fname_disk_to_usr. I guess it will need to be...

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  2020-09-08  4:57   ` Eric Biggers
  2020-09-09 12:20     ` Jeff Layton
@ 2020-09-09 15:53     ` Jeff Layton
  2020-09-09 16:33       ` Eric Biggers
  1 sibling, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 15:53 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

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,
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it
  2020-09-08 22:53       ` Eric Biggers
@ 2020-09-09 16:02         ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 16:02 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt, Xiubo Li

On Tue, 2020-09-08 at 15:53 -0700, Eric Biggers wrote:
> On Tue, Sep 08, 2020 at 08:50:04AM -0400, Jeff Layton wrote:
> > > > +EXPORT_SYMBOL(fscrypt_encode_nokey_name);
> > > 
> > > Why does this need to be exported?
> > > 
> > > There's no user of this function introduced in this patchset.
> > > 
> > > - Eric
> > 
> > Yeah, I probably should have dropped this from the series for now as
> > nothing uses it yet, but eventually we may need this. I did a fairly
> > detailed writeup of the problem here:
> > 
> >     https://tracker.ceph.com/issues/47162
> > 
> > Basically, we still need to allow clients to look up dentries in the MDS
> > even when they don't have the key.
> > 
> > There are a couple of different approaches, but the simplest is to just
> > have the client always store long dentry names using the nokey_name, and
> > then keep the full name in a new field in the dentry representation that
> > is sent across the wire.
> > 
> > This requires some changes to the Ceph MDS (which is what that tracker
> > bug is about), and will mean enshrining the nokey name in perpetuity.
> > We're still looking at this for now though, and we're open to other
> > approaches if you've got any to suggest.
> 
> The (persistent) directory entries have to include the full ciphertext
> filenames.  If they only included the no-key names, then it wouldn't always be
> possible to translate them back into the original plaintext filenames.
> 
> It's also required that the filesystem can find a specific directory entry given
> its corresponding no-key name.  For a network filesystem, that can be done
> either on the client (request all filenames in the directory, then check all of
> them...), or on the server (give it the no-key name and have it do the matching;
> it would need to know the specifics of how the no-key names work).
> 
> The no-key names aren't currently stable, and it would be nice to keep them that
> way since we might want to improve how they work later.  But if you need to
> stabilize a version of the no-key name format for use in the ceph protocol so
> that the server can do the matching, it would be possible to do that.  It
> wouldn't even necessarily have to be what fscrypt currently uses; e.g. if it
> were to simplify things a lot for you to simply use SHA-256(ciphertext_name)
> instead of the current 'struct fscrypt_nokey_name', you could do that.
> 

(cc'ing Xiubo since he's working on the MDS part)

We probably will need to make a stable representation. I think the nokey
name scheme as it stands would be fine for this purpose, particularly as
the representation is only different for really long filenames. We'd
only need to carry an alternate name for dentries with names longer than
~150 chars, and those are somewhat rare.

Much of this depends on the MDS though, and I'm a lot less familiar with
that part. So for now, no need to do anything. We'll reach out once we
have a more solid plan of how we want to handle this.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-09 10:47         ` Jeff Layton
@ 2020-09-09 16:12           ` Eric Biggers
  2020-09-09 16:51             ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-09 16:12 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Wed, Sep 09, 2020 at 06:47:28AM -0400, Jeff Layton wrote:
> On Tue, 2020-09-08 at 15:31 -0700, Eric Biggers wrote:
> > On Tue, Sep 08, 2020 at 07:27:58AM -0400, Jeff Layton wrote:
> > > On Mon, 2020-09-07 at 20:38 -0700, Eric Biggers wrote:
> > > > On Fri, Sep 04, 2020 at 12:05:20PM -0400, Jeff Layton wrote:
> > > > > Ceph needs to be able to allocate inodes ahead of a create that might
> > > > > involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
> > > > > but it puts the inode on the sb->s_inodes list, and we don't want to
> > > > > do that until we're ready to insert it into the hash.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/inode.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/fs/inode.c b/fs/inode.c
> > > > > index 72c4c347afb7..61554c2477ab 100644
> > > > > --- a/fs/inode.c
> > > > > +++ b/fs/inode.c
> > > > > @@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
> > > > >  	}
> > > > >  	return inode;
> > > > >  }
> > > > > +EXPORT_SYMBOL(new_inode_pseudo);
> > > > >  
> > > > 
> > > > What's the problem with putting the new inode on sb->s_inodes already?
> > > > That's what all the other filesystems do.
> > > > 
> > > 
> > > The existing ones are all local filesystems that use
> > > insert_inode_locked() and similar paths. Ceph needs to use the '5'
> > > variants of those functions (inode_insert5(), iget5_locked(), etc.).
> > > 
> > > When we go to insert it into the hash in inode_insert5(), we'd need to
> > > set I_CREATING if allocated from new_inode(). But, if you do _that_,
> > > then you'll get back ESTALE from find_inode() if (eg.) someone calls
> > > iget5_locked() before you can clear I_CREATING.
> > > 
> > > Hitting that race is easy with an asynchronous create. The simplest
> > > scheme to avoid that is to just export new_inode_pseudo and keep it off
> > > of s_inodes until we're ready to do the insert. The only real issue here
> > > is that this inode won't be findable by evict_inodes during umount, but
> > > that shouldn't be happening during an active syscall anyway.
> > 
> > Is your concern the following scenario?
> > 
> > 1. ceph successfully created a new file on the server
> > 2. inode_insert5() is called for the new file's inode
> > 3. error occurs in ceph_fill_inode()
> > 4. discard_new_inode() is called
> > 5. another thread looks up the inode and gets ESTALE
> > 6. iput() is finally called
> > 
> > And the claim is that the ESTALE in (5) is unexpected?  I'm not sure that it's
> > unexpected, given that the file allegedly failed to be created...  Also, it
> > seems that maybe (3) isn't something that should actually happen, since after
> > (1) it's too late to fail the file creation.
> > 
> 
> No, more like:
> 
> Syscall					Workqueue
> ------------------------------------------------------------------------------
> 1. allocate an inode
> 2. determine we can do an async create
>    and allocate an inode number for it
> 3. hash the inode (must set I_CREATING
>    if we allocated with new_inode()) 
> 4. issue the call to the MDS
> 5. finish filling out the inode()
> 6.					MDS reply comes in, and workqueue thread
> 					looks up new inode (-ESTALE)
> 7. unlock_new_inode()
> 
> 
> Because 6 happens before 7 in this case, we get an ESTALE on that
> lookup.

How is ESTALE at (6) possible?  (3) will set I_NEW on the inode when inserting
it into the inode hash table.  Then (6) will wait for I_NEW to be cleared before
returning the inode.  (7) will clear I_NEW and I_CREATING together.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries
  2020-09-09 12:26     ` Jeff Layton
@ 2020-09-09 16:18       ` Eric Biggers
  0 siblings, 0 replies; 59+ messages in thread
From: Eric Biggers @ 2020-09-09 16:18 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Wed, Sep 09, 2020 at 08:26:51AM -0400, Jeff Layton wrote:
> On Mon, 2020-09-07 at 22:12 -0700, Eric Biggers wrote:
> > On Fri, Sep 04, 2020 at 12:05:34PM -0400, Jeff Layton wrote:
> > > If we have an encrypted dentry, then we need to test whether a new key
> > > might have been established or removed. Do that before we test anything
> > > else about the dentry.
> > 
> > A more accurate explanation would be:
> > 
> > "If we have a dentry which represents a no-key name, then we need to test
> >  whether the parent directory's encryption key has since been added."
> > 
> 
> Can't a key also be removed (e.g. fscrypt lock /path/to/dir)?
> 
> Does that result in the dentries below that point being invalidated?

It results in the dentries (and inodes) being evicted, not invalidated.
See try_to_lock_encrypted_files() in fs/crypto/keyring.c.

So, fscrypt_d_revalidate() doesn't need to consider key removal.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  2020-09-09 15:53     ` Jeff Layton
@ 2020-09-09 16:33       ` Eric Biggers
  2020-09-09 17:19         ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-09 16:33 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

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

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-09 16:12           ` Eric Biggers
@ 2020-09-09 16:51             ` Jeff Layton
  2020-09-09 18:49               ` Eric Biggers
  0 siblings, 1 reply; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 16:51 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

On Wed, 2020-09-09 at 09:12 -0700, Eric Biggers wrote:
> On Wed, Sep 09, 2020 at 06:47:28AM -0400, Jeff Layton wrote:
> > On Tue, 2020-09-08 at 15:31 -0700, Eric Biggers wrote:
> > > On Tue, Sep 08, 2020 at 07:27:58AM -0400, Jeff Layton wrote:
> > > > On Mon, 2020-09-07 at 20:38 -0700, Eric Biggers wrote:
> > > > > On Fri, Sep 04, 2020 at 12:05:20PM -0400, Jeff Layton wrote:
> > > > > > Ceph needs to be able to allocate inodes ahead of a create that might
> > > > > > involve a fscrypt-encrypted inode. new_inode() almost fits the bill,
> > > > > > but it puts the inode on the sb->s_inodes list, and we don't want to
> > > > > > do that until we're ready to insert it into the hash.
> > > > > > 
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > ---
> > > > > >  fs/inode.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > > 
> > > > > > diff --git a/fs/inode.c b/fs/inode.c
> > > > > > index 72c4c347afb7..61554c2477ab 100644
> > > > > > --- a/fs/inode.c
> > > > > > +++ b/fs/inode.c
> > > > > > @@ -935,6 +935,7 @@ struct inode *new_inode_pseudo(struct super_block *sb)
> > > > > >  	}
> > > > > >  	return inode;
> > > > > >  }
> > > > > > +EXPORT_SYMBOL(new_inode_pseudo);
> > > > > >  
> > > > > 
> > > > > What's the problem with putting the new inode on sb->s_inodes already?
> > > > > That's what all the other filesystems do.
> > > > > 
> > > > 
> > > > The existing ones are all local filesystems that use
> > > > insert_inode_locked() and similar paths. Ceph needs to use the '5'
> > > > variants of those functions (inode_insert5(), iget5_locked(), etc.).
> > > > 
> > > > When we go to insert it into the hash in inode_insert5(), we'd need to
> > > > set I_CREATING if allocated from new_inode(). But, if you do _that_,
> > > > then you'll get back ESTALE from find_inode() if (eg.) someone calls
> > > > iget5_locked() before you can clear I_CREATING.
> > > > 
> > > > Hitting that race is easy with an asynchronous create. The simplest
> > > > scheme to avoid that is to just export new_inode_pseudo and keep it off
> > > > of s_inodes until we're ready to do the insert. The only real issue here
> > > > is that this inode won't be findable by evict_inodes during umount, but
> > > > that shouldn't be happening during an active syscall anyway.
> > > 
> > > Is your concern the following scenario?
> > > 
> > > 1. ceph successfully created a new file on the server
> > > 2. inode_insert5() is called for the new file's inode
> > > 3. error occurs in ceph_fill_inode()
> > > 4. discard_new_inode() is called
> > > 5. another thread looks up the inode and gets ESTALE
> > > 6. iput() is finally called
> > > 
> > > And the claim is that the ESTALE in (5) is unexpected?  I'm not sure that it's
> > > unexpected, given that the file allegedly failed to be created...  Also, it
> > > seems that maybe (3) isn't something that should actually happen, since after
> > > (1) it's too late to fail the file creation.
> > > 
> > 
> > No, more like:
> > 
> > Syscall					Workqueue
> > ------------------------------------------------------------------------------
> > 1. allocate an inode
> > 2. determine we can do an async create
> >    and allocate an inode number for it
> > 3. hash the inode (must set I_CREATING
> >    if we allocated with new_inode()) 
> > 4. issue the call to the MDS
> > 5. finish filling out the inode()
> > 6.					MDS reply comes in, and workqueue thread
> > 					looks up new inode (-ESTALE)
> > 7. unlock_new_inode()
> > 
> > 
> > Because 6 happens before 7 in this case, we get an ESTALE on that
> > lookup.
> 
> How is ESTALE at (6) possible?  (3) will set I_NEW on the inode when inserting
> it into the inode hash table.  Then (6) will wait for I_NEW to be cleared before
> returning the inode.  (7) will clear I_NEW and I_CREATING together.
> 

Long call chain, but:

ceph_fill_trace
   ceph_get_inode
      iget5_locked
         ilookup5(..._nowait, etc)
            find_inode_fast


...and find_inode_fast does:

                if (unlikely(inode->i_state & I_CREATING)) {                                        
                        spin_unlock(&inode->i_lock);                                                
                        return ERR_PTR(-ESTALE);                                                    
                }                                                                                   

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr
  2020-09-09 16:33       ` Eric Biggers
@ 2020-09-09 17:19         ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 17:19 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt

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!
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-09 16:51             ` Jeff Layton
@ 2020-09-09 18:49               ` Eric Biggers
  2020-09-09 19:24                 ` Jeff Layton
  0 siblings, 1 reply; 59+ messages in thread
From: Eric Biggers @ 2020-09-09 18:49 UTC (permalink / raw)
  To: Jeff Layton; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt, Al Viro

[+Cc Al]

On Wed, Sep 09, 2020 at 12:51:02PM -0400, Jeff Layton wrote:
> > > No, more like:
> > > 
> > > Syscall					Workqueue
> > > ------------------------------------------------------------------------------
> > > 1. allocate an inode
> > > 2. determine we can do an async create
> > >    and allocate an inode number for it
> > > 3. hash the inode (must set I_CREATING
> > >    if we allocated with new_inode()) 
> > > 4. issue the call to the MDS
> > > 5. finish filling out the inode()
> > > 6.					MDS reply comes in, and workqueue thread
> > > 					looks up new inode (-ESTALE)
> > > 7. unlock_new_inode()
> > > 
> > > 
> > > Because 6 happens before 7 in this case, we get an ESTALE on that
> > > lookup.
> > 
> > How is ESTALE at (6) possible?  (3) will set I_NEW on the inode when inserting
> > it into the inode hash table.  Then (6) will wait for I_NEW to be cleared before
> > returning the inode.  (7) will clear I_NEW and I_CREATING together.
> > 
> 
> Long call chain, but:
> 
> ceph_fill_trace
>    ceph_get_inode
>       iget5_locked
>          ilookup5(..._nowait, etc)
>             find_inode_fast
> 
> 
> ...and find_inode_fast does:
> 
>                 if (unlikely(inode->i_state & I_CREATING)) {                                        
>                         spin_unlock(&inode->i_lock);                                                
>                         return ERR_PTR(-ESTALE);                                                    
>                 }                                                                                   

Why does ilookup5() not wait for I_NEW to be cleared if the inode has
I_NEW|I_CREATING, but it does wait for I_NEW to be cleared if I_NEW is set its
own?  That seems like a bug.

- Eric

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [RFC PATCH v2 01/18] vfs: export new_inode_pseudo
  2020-09-09 18:49               ` Eric Biggers
@ 2020-09-09 19:24                 ` Jeff Layton
  0 siblings, 0 replies; 59+ messages in thread
From: Jeff Layton @ 2020-09-09 19:24 UTC (permalink / raw)
  To: Eric Biggers; +Cc: ceph-devel, linux-fsdevel, linux-fscrypt, Al Viro

On Wed, 2020-09-09 at 11:49 -0700, Eric Biggers wrote:
> [+Cc Al]
> 
> On Wed, Sep 09, 2020 at 12:51:02PM -0400, Jeff Layton wrote:
> > > > No, more like:
> > > > 
> > > > Syscall					Workqueue
> > > > ------------------------------------------------------------------------------
> > > > 1. allocate an inode
> > > > 2. determine we can do an async create
> > > >    and allocate an inode number for it
> > > > 3. hash the inode (must set I_CREATING
> > > >    if we allocated with new_inode()) 
> > > > 4. issue the call to the MDS
> > > > 5. finish filling out the inode()
> > > > 6.					MDS reply comes in, and workqueue thread
> > > > 					looks up new inode (-ESTALE)
> > > > 7. unlock_new_inode()
> > > > 
> > > > 
> > > > Because 6 happens before 7 in this case, we get an ESTALE on that
> > > > lookup.
> > > 
> > > How is ESTALE at (6) possible?  (3) will set I_NEW on the inode when inserting
> > > it into the inode hash table.  Then (6) will wait for I_NEW to be cleared before
> > > returning the inode.  (7) will clear I_NEW and I_CREATING together.
> > > 
> > 
> > Long call chain, but:
> > 
> > ceph_fill_trace
> >    ceph_get_inode
> >       iget5_locked
> >          ilookup5(..._nowait, etc)
> >             find_inode_fast
> > 
> > 
> > ...and find_inode_fast does:
> > 
> >                 if (unlikely(inode->i_state & I_CREATING)) {                                        
> >                         spin_unlock(&inode->i_lock);                                                
> >                         return ERR_PTR(-ESTALE);                                                    
> >                 }                                                                                   
> 
> Why does ilookup5() not wait for I_NEW to be cleared if the inode has
> I_NEW|I_CREATING, but it does wait for I_NEW to be cleared if I_NEW is set its
> own?  That seems like a bug.
> 

Funny, I asked Al the same thing on IRC the other day:

23:28 < jlayton> viro: wondering if there is a potential race with I_CREATING in find_inode. 
                 Seems like you could have 2 tasks racing in calls to iget5_locked for the 
                 same inode. One creates an inode and starts instantiating it, and the second 
                 one gets NULL back because I_CREATING is set.
23:30 < viro> jlayton: where would I_CREATING come from?
23:30 < viro> it's set on insert_inode_locked() and similar paths
23:31 < viro> where you want iget5_locked() to fuck off and eat ESTALE
23:31 < jlayton> ok, right -- I was trying to pass an inode from new_inode to inode_insert5
23:32 < viro> seeing that it's been asked for an inode number that did _not_ exist until just 
              now (we'd just allocated it)

The assumption is that we'll never go looking for an inode until after
I_NEW is cleared. In the case of an asynchronous create in ceph though,
we may do exactly that if the reply comes back very quickly.

-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 59+ messages in thread

end of thread, other threads:[~2020-09-09 19:24 UTC | newest]

Thread overview: 59+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 01/18] vfs: export new_inode_pseudo Jeff Layton
2020-09-08  3:38   ` Eric Biggers
2020-09-08 11:27     ` Jeff Layton
2020-09-08 22:31       ` Eric Biggers
2020-09-09 10:47         ` Jeff Layton
2020-09-09 16:12           ` Eric Biggers
2020-09-09 16:51             ` Jeff Layton
2020-09-09 18:49               ` Eric Biggers
2020-09-09 19:24                 ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 02/18] fscrypt: drop unused inode argument from fscrypt_fname_alloc_buffer Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 03/18] fscrypt: export fscrypt_d_revalidate Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode Jeff Layton
2020-09-08  3:48   ` Eric Biggers
2020-09-08 11:29     ` Jeff Layton
2020-09-08 12:29     ` Jeff Layton
2020-09-08 22:34       ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted Jeff Layton
2020-09-08  3:52   ` Eric Biggers
2020-09-08 12:54     ` Jeff Layton
2020-09-08 23:08       ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it Jeff Layton
2020-09-08  3:55   ` Eric Biggers
2020-09-08 12:50     ` Jeff Layton
2020-09-08 22:53       ` Eric Biggers
2020-09-09 16:02         ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/ Jeff Layton
2020-09-08  3:59   ` Eric Biggers
2020-09-08 12:51     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 08/18] ceph: add fscrypt ioctls Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 09/18] ceph: crypto context handling for ceph Jeff Layton
2020-09-08  4:29   ` Eric Biggers
2020-09-08 16:14     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 10/18] ceph: preallocate inode for ops that may create one Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC Jeff Layton
2020-09-08  4:43   ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr Jeff Layton
2020-09-08  4:57   ` Eric Biggers
2020-09-09 12:20     ` Jeff Layton
2020-09-09 15:53     ` Jeff Layton
2020-09-09 16:33       ` Eric Biggers
2020-09-09 17:19         ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 13/18] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
2020-09-08  5:06   ` Eric Biggers
2020-09-09 12:24     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
2020-09-08  5:12   ` Eric Biggers
2020-09-09 12:26     ` Jeff Layton
2020-09-09 16:18       ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames Jeff Layton
2020-09-08  5:34   ` Eric Biggers
2020-09-09 13:02     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 17/18] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
2020-09-04 16:11   ` Jeff Layton
2020-09-08  5:43   ` Eric Biggers
2020-09-08  5:54 ` [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Eric Biggers
2020-09-08 12:09   ` Jeff Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).