linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/13] fscrypt: improve file creation flow
@ 2020-09-17  4:11 Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 01/13] fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
                   ` (13 more replies)
  0 siblings, 14 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

Hello,

This series reworks the implementation of creating new encrypted files
by introducing new helper functions that allow filesystems to set up the
inodes' keys earlier, prior to taking too many filesystem locks.

This fixes deadlocks that are possible during memory reclaim because
fscrypt_get_encryption_info() isn't GFP_NOFS-safe, yet it's called
during an ext4 transaction or under f2fs_lock_op().  It also fixes a
similar deadlock where f2fs can try to recursively lock a page when the
test_dummy_encryption mount option is in use.

It also solves an ordering problem that the ceph support for fscrypt
will have.  For more details about this ordering problem, see the
discussion on Jeff Layton's RFC patchsets for ceph fscrypt support
(v1: https://lkml.kernel.org/linux-fscrypt/20200821182813.52570-1-jlayton@kernel.org/T/#u
 v2: https://lkml.kernel.org/linux-fscrypt/20200904160537.76663-1-jlayton@kernel.org/T/#u
 v3: https://lkml.kernel.org/linux-fscrypt/20200914191707.380444-1-jlayton@kernel.org/T/#u)
Note that v3 of the ceph patchset is based on v2 of this patchset.

Patch 1 adds the above-mentioned new helper functions.  Patches 2-5
convert ext4, f2fs, and ubifs to use them, and patches 6-9 clean up a
few things afterwards.

Finally, patches 10-13 change the implementation of
test_dummy_encryption to no longer set up an encryption key for
unencrypted directories, which was confusing and was causing problems.

This patchset applies to the master branch of
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git.
It can also be retrieved from tag "fscrypt-file-creation-v3" of
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git.

I'm looking to apply this for 5.10; reviews are greatly appreciated!

Changed v2 => v3:
  - Added patch that changes fscrypt_set_test_dummy_encryption() to take
    a 'const char *'.  (Needed by ceph.)
  - Fixed bug where fscrypt_prepare_new_inode() succeeded even if the
    new inode's key couldn't be set up.
  - Fixed bug where fscrypt_prepare_new_inode() wouldn't derive the
    dirhash key for new casefolded directories.
  - Made warning messages account for i_ino possibly being 0 now.

Changed v1 => v2:
  - Added mention of another deadlock this fixes.
  - Added patches to improve the test_dummy_encryption implementation.
  - Dropped an ext4 cleanup patch that can be done separately later.
  - Lots of small cleanups, and a couple small fixes.

Eric Biggers (13):
  fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()
  ext4: factor out ext4_xattr_credits_for_new_inode()
  ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context()
  f2fs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
  ubifs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
  fscrypt: adjust logging for in-creation inodes
  fscrypt: remove fscrypt_inherit_context()
  fscrypt: require that fscrypt_encrypt_symlink() already has key
  fscrypt: stop pretending that key setup is nofs-safe
  fscrypt: make "#define fscrypt_policy" user-only
  fscrypt: move fscrypt_prepare_symlink() out-of-line
  fscrypt: handle test_dummy_encryption in more logical way
  fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char
    *'

 fs/crypto/crypto.c           |   4 +-
 fs/crypto/fname.c            |  11 +-
 fs/crypto/fscrypt_private.h  |  10 +-
 fs/crypto/hooks.c            |  65 ++++++++----
 fs/crypto/inline_crypt.c     |   7 +-
 fs/crypto/keyring.c          |   9 +-
 fs/crypto/keysetup.c         | 182 +++++++++++++++++++++++--------
 fs/crypto/keysetup_v1.c      |   8 +-
 fs/crypto/policy.c           | 200 ++++++++++++++++++++---------------
 fs/ext4/ext4.h               |   6 +-
 fs/ext4/ialloc.c             | 119 +++++++++++----------
 fs/ext4/super.c              |  17 +--
 fs/f2fs/dir.c                |   2 +-
 fs/f2fs/f2fs.h               |  25 +----
 fs/f2fs/namei.c              |   7 +-
 fs/f2fs/super.c              |  16 +--
 fs/ubifs/dir.c               |  38 +++----
 include/linux/fscrypt.h      | 120 +++++++--------------
 include/uapi/linux/fscrypt.h |   6 +-
 19 files changed, 474 insertions(+), 378 deletions(-)


base-commit: 5e895bd4d5233cb054447d0491d4e63c8496d419
-- 
2.28.0


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

* [PATCH v3 01/13] fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 02/13] ext4: factor out ext4_xattr_credits_for_new_inode() Eric Biggers
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

fscrypt_get_encryption_info() is intended to be GFP_NOFS-safe.  But
actually it isn't, since it uses functions like crypto_alloc_skcipher()
which aren't GFP_NOFS-safe, even when called under memalloc_nofs_save().
Therefore it can deadlock when called from a context that needs
GFP_NOFS, e.g. during an ext4 transaction or between f2fs_lock_op() and
f2fs_unlock_op().  This happens when creating a new encrypted file.

We can't fix this by just not setting up the key for new inodes right
away, since new symlinks need their key to encrypt the symlink target.

So we need to set up the new inode's key before starting the
transaction.  But just calling fscrypt_get_encryption_info() earlier
doesn't work, since it assumes the encryption context is already set,
and the encryption context can't be set until the transaction.

The recently proposed fscrypt support for the ceph filesystem
(https://lkml.kernel.org/linux-fscrypt/20200821182813.52570-1-jlayton@kernel.org/T/#u)
will have this same ordering problem too, since ceph will need to
encrypt new symlinks before setting their encryption context.

Finally, f2fs can deadlock when the filesystem is mounted with
'-o test_dummy_encryption' and a new file is created in an existing
unencrypted directory.  Similarly, this is caused by holding too many
locks when calling fscrypt_get_encryption_info().

To solve all these problems, add new helper functions:

- fscrypt_prepare_new_inode() sets up a new inode's encryption key
  (fscrypt_info), using the parent directory's encryption policy and a
  new random nonce.  It neither reads nor writes the encryption context.

- fscrypt_set_context() persists the encryption context of a new inode,
  using the information from the fscrypt_info already in memory.  This
  replaces fscrypt_inherit_context().

Temporarily keep fscrypt_inherit_context() around until all filesystems
have been converted to use fscrypt_set_context().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h |   3 +
 fs/crypto/keysetup.c        | 195 +++++++++++++++++++++++++++---------
 fs/crypto/policy.c          |  62 ++++++++++--
 include/linux/fscrypt.h     |  17 ++++
 4 files changed, 223 insertions(+), 54 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8117a61b6f558..355f6d9377517 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -572,6 +572,9 @@ int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key);
 int fscrypt_derive_dirhash_key(struct fscrypt_info *ci,
 			       const struct fscrypt_master_key *mk);
 
+void fscrypt_hash_inode_number(struct fscrypt_info *ci,
+			       const struct fscrypt_master_key *mk);
+
 /* keysetup_v1.c */
 
 void fscrypt_put_direct_key(struct fscrypt_direct_key *dk);
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index fea6226afc2b0..6159168972146 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -10,6 +10,7 @@
 
 #include <crypto/skcipher.h>
 #include <linux/key.h>
+#include <linux/random.h>
 
 #include "fscrypt_private.h"
 
@@ -222,6 +223,16 @@ int fscrypt_derive_dirhash_key(struct fscrypt_info *ci,
 	return 0;
 }
 
+void fscrypt_hash_inode_number(struct fscrypt_info *ci,
+			       const struct fscrypt_master_key *mk)
+{
+	WARN_ON(ci->ci_inode->i_ino == 0);
+	WARN_ON(!mk->mk_ino_hash_key_initialized);
+
+	ci->ci_hashed_ino = (u32)siphash_1u64(ci->ci_inode->i_ino,
+					      &mk->mk_ino_hash_key);
+}
+
 static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
 					    struct fscrypt_master_key *mk)
 {
@@ -254,13 +265,20 @@ static int fscrypt_setup_iv_ino_lblk_32_key(struct fscrypt_info *ci,
 			return err;
 	}
 
-	ci->ci_hashed_ino = (u32)siphash_1u64(ci->ci_inode->i_ino,
-					      &mk->mk_ino_hash_key);
+	/*
+	 * New inodes may not have an inode number assigned yet.
+	 * Hashing their inode number is delayed until later.
+	 */
+	if (ci->ci_inode->i_ino == 0)
+		WARN_ON(!(ci->ci_inode->i_state & I_CREATING));
+	else
+		fscrypt_hash_inode_number(ci, mk);
 	return 0;
 }
 
 static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
-				     struct fscrypt_master_key *mk)
+				     struct fscrypt_master_key *mk,
+				     bool need_dirhash_key)
 {
 	int err;
 
@@ -306,7 +324,7 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
 		return err;
 
 	/* Derive a secret dirhash key for directories that need it. */
-	if (S_ISDIR(ci->ci_inode->i_mode) && IS_CASEFOLDED(ci->ci_inode)) {
+	if (need_dirhash_key) {
 		err = fscrypt_derive_dirhash_key(ci, mk);
 		if (err)
 			return err;
@@ -326,6 +344,7 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
  * key being removed with a new inode starting to use it.
  */
 static int setup_file_encryption_key(struct fscrypt_info *ci,
+				     bool need_dirhash_key,
 				     struct key **master_key_ret)
 {
 	struct key *key;
@@ -400,7 +419,7 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
 		err = fscrypt_setup_v1_file_key(ci, mk->mk_secret.raw);
 		break;
 	case FSCRYPT_POLICY_V2:
-		err = fscrypt_setup_v2_file_key(ci, mk);
+		err = fscrypt_setup_v2_file_key(ci, mk, need_dirhash_key);
 		break;
 	default:
 		WARN_ON(1);
@@ -454,57 +473,28 @@ static void put_crypt_info(struct fscrypt_info *ci)
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
 
-int fscrypt_get_encryption_info(struct inode *inode)
+static int
+fscrypt_setup_encryption_info(struct inode *inode,
+			      const union fscrypt_policy *policy,
+			      const u8 nonce[FSCRYPT_FILE_NONCE_SIZE],
+			      bool need_dirhash_key)
 {
 	struct fscrypt_info *crypt_info;
-	union fscrypt_context ctx;
 	struct fscrypt_mode *mode;
 	struct key *master_key = NULL;
 	int res;
 
-	if (fscrypt_has_encryption_key(inode))
-		return 0;
-
 	res = fscrypt_initialize(inode->i_sb->s_cop->flags);
 	if (res)
 		return res;
 
-	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
-	if (res < 0) {
-		const union fscrypt_context *dummy_ctx =
-			fscrypt_get_dummy_context(inode->i_sb);
-
-		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
-			fscrypt_warn(inode,
-				     "Error %d getting encryption context",
-				     res);
-			return res;
-		}
-		/* Fake up a context for an unencrypted directory */
-		res = fscrypt_context_size(dummy_ctx);
-		memcpy(&ctx, dummy_ctx, res);
-	}
-
 	crypt_info = kmem_cache_zalloc(fscrypt_info_cachep, GFP_NOFS);
 	if (!crypt_info)
 		return -ENOMEM;
 
 	crypt_info->ci_inode = inode;
-
-	res = fscrypt_policy_from_context(&crypt_info->ci_policy, &ctx, res);
-	if (res) {
-		fscrypt_warn(inode,
-			     "Unrecognized or corrupt encryption context");
-		goto out;
-	}
-
-	memcpy(crypt_info->ci_nonce, fscrypt_context_nonce(&ctx),
-	       FSCRYPT_FILE_NONCE_SIZE);
-
-	if (!fscrypt_supported_policy(&crypt_info->ci_policy, inode)) {
-		res = -EINVAL;
-		goto out;
-	}
+	crypt_info->ci_policy = *policy;
+	memcpy(crypt_info->ci_nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
 
 	mode = select_encryption_mode(&crypt_info->ci_policy, inode);
 	if (IS_ERR(mode)) {
@@ -514,13 +504,14 @@ int fscrypt_get_encryption_info(struct inode *inode)
 	WARN_ON(mode->ivsize > FSCRYPT_MAX_IV_SIZE);
 	crypt_info->ci_mode = mode;
 
-	res = setup_file_encryption_key(crypt_info, &master_key);
+	res = setup_file_encryption_key(crypt_info, need_dirhash_key,
+					&master_key);
 	if (res)
 		goto out;
 
 	/*
-	 * Multiple tasks may race to set ->i_crypt_info, so use
-	 * cmpxchg_release().  This pairs with the smp_load_acquire() in
+	 * For existing inodes, multiple tasks may race to set ->i_crypt_info.
+	 * So use cmpxchg_release().  This pairs with the smp_load_acquire() in
 	 * fscrypt_get_info().  I.e., here we publish ->i_crypt_info with a
 	 * RELEASE barrier so that other tasks can ACQUIRE it.
 	 */
@@ -550,13 +541,127 @@ int fscrypt_get_encryption_info(struct inode *inode)
 		up_read(&mk->mk_secret_sem);
 		key_put(master_key);
 	}
+	put_crypt_info(crypt_info);
+	return res;
+}
+
+/**
+ * fscrypt_get_encryption_info() - set up an inode's encryption key
+ * @inode: the inode to set up the key for
+ *
+ * Set up ->i_crypt_info, if it hasn't already been done.
+ *
+ * Note: unless ->i_crypt_info is already set, this isn't %GFP_NOFS-safe.  So
+ * generally this shouldn't be called from within a filesystem transaction.
+ *
+ * Return: 0 if ->i_crypt_info was set or was already set, *or* if the
+ *	   encryption key is unavailable.  (Use fscrypt_has_encryption_key() to
+ *	   distinguish these cases.)  Also can return another -errno code.
+ */
+int fscrypt_get_encryption_info(struct inode *inode)
+{
+	int res;
+	union fscrypt_context ctx;
+	union fscrypt_policy policy;
+
+	if (fscrypt_has_encryption_key(inode))
+		return 0;
+
+	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
+	if (res < 0) {
+		const union fscrypt_context *dummy_ctx =
+			fscrypt_get_dummy_context(inode->i_sb);
+
+		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
+			fscrypt_warn(inode,
+				     "Error %d getting encryption context",
+				     res);
+			return res;
+		}
+		/* Fake up a context for an unencrypted directory */
+		res = fscrypt_context_size(dummy_ctx);
+		memcpy(&ctx, dummy_ctx, res);
+	}
+
+	res = fscrypt_policy_from_context(&policy, &ctx, res);
+	if (res) {
+		fscrypt_warn(inode,
+			     "Unrecognized or corrupt encryption context");
+		return res;
+	}
+
+	if (!fscrypt_supported_policy(&policy, inode))
+		return -EINVAL;
+
+	res = fscrypt_setup_encryption_info(inode, &policy,
+					    fscrypt_context_nonce(&ctx),
+					    IS_CASEFOLDED(inode) &&
+					    S_ISDIR(inode->i_mode));
 	if (res == -ENOKEY)
 		res = 0;
-	put_crypt_info(crypt_info);
 	return res;
 }
 EXPORT_SYMBOL(fscrypt_get_encryption_info);
 
+/**
+ * fscrypt_prepare_new_inode() - prepare to create a new inode in a directory
+ * @dir: a possibly-encrypted directory
+ * @inode: the new inode.  ->i_mode must be set already.
+ *	   ->i_ino doesn't need to be set yet.
+ * @encrypt_ret: (output) set to %true if the new inode will be encrypted
+ *
+ * If the directory is encrypted, set up its ->i_crypt_info in preparation for
+ * encrypting the name of the new file.  Also, if the new inode will be
+ * encrypted, set up its ->i_crypt_info and set *encrypt_ret=true.
+ *
+ * This isn't %GFP_NOFS-safe, and therefore it should be called before starting
+ * any filesystem transaction to create the inode.  For this reason, ->i_ino
+ * isn't required to be set yet, as the filesystem may not have set it yet.
+ *
+ * This doesn't persist the new inode's encryption context.  That still needs to
+ * be done later by calling fscrypt_set_context().
+ *
+ * Return: 0 on success, -ENOKEY if the encryption key is missing, or another
+ *	   -errno code
+ */
+int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
+			      bool *encrypt_ret)
+{
+	int err;
+	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
+
+	if (!IS_ENCRYPTED(dir) && fscrypt_get_dummy_context(dir->i_sb) == NULL)
+		return 0;
+
+	err = fscrypt_get_encryption_info(dir);
+	if (err)
+		return err;
+	if (!fscrypt_has_encryption_key(dir))
+		return -ENOKEY;
+
+	if (WARN_ON_ONCE(inode->i_mode == 0))
+		return -EINVAL;
+
+	/*
+	 * Only regular files, directories, and symlinks are encrypted.
+	 * Special files like device nodes and named pipes aren't.
+	 */
+	if (!S_ISREG(inode->i_mode) &&
+	    !S_ISDIR(inode->i_mode) &&
+	    !S_ISLNK(inode->i_mode))
+		return 0;
+
+	*encrypt_ret = true;
+
+	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
+	return fscrypt_setup_encryption_info(inode,
+					     &dir->i_crypt_info->ci_policy,
+					     nonce,
+					     IS_CASEFOLDED(dir) &&
+					     S_ISDIR(inode->i_mode));
+}
+EXPORT_SYMBOL_GPL(fscrypt_prepare_new_inode);
+
 /**
  * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
  * @inode: an inode being evicted
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index b92f345231780..7e96953d385ec 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -236,18 +236,19 @@ bool fscrypt_supported_policy(const union fscrypt_policy *policy_u,
 }
 
 /**
- * fscrypt_new_context_from_policy() - create a new fscrypt_context from
- *				       an fscrypt_policy
+ * fscrypt_new_context() - create a new fscrypt_context
  * @ctx_u: output context
  * @policy_u: input policy
+ * @nonce: nonce to use
  *
  * Create an fscrypt_context for an inode that is being assigned the given
- * encryption policy.  A new nonce is randomly generated.
+ * encryption policy.  @nonce must be a new random nonce.
  *
  * Return: the size of the new context in bytes.
  */
-static int fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
-					   const union fscrypt_policy *policy_u)
+static int fscrypt_new_context(union fscrypt_context *ctx_u,
+			       const union fscrypt_policy *policy_u,
+			       const u8 nonce[FSCRYPT_FILE_NONCE_SIZE])
 {
 	memset(ctx_u, 0, sizeof(*ctx_u));
 
@@ -265,7 +266,7 @@ static int fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
 		memcpy(ctx->master_key_descriptor,
 		       policy->master_key_descriptor,
 		       sizeof(ctx->master_key_descriptor));
-		get_random_bytes(ctx->nonce, sizeof(ctx->nonce));
+		memcpy(ctx->nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
 		return sizeof(*ctx);
 	}
 	case FSCRYPT_POLICY_V2: {
@@ -281,7 +282,7 @@ static int fscrypt_new_context_from_policy(union fscrypt_context *ctx_u,
 		memcpy(ctx->master_key_identifier,
 		       policy->master_key_identifier,
 		       sizeof(ctx->master_key_identifier));
-		get_random_bytes(ctx->nonce, sizeof(ctx->nonce));
+		memcpy(ctx->nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
 		return sizeof(*ctx);
 	}
 	}
@@ -377,6 +378,7 @@ static int fscrypt_get_policy(struct inode *inode, union fscrypt_policy *policy)
 static int set_encryption_policy(struct inode *inode,
 				 const union fscrypt_policy *policy)
 {
+	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
 	union fscrypt_context ctx;
 	int ctxsize;
 	int err;
@@ -414,7 +416,8 @@ static int set_encryption_policy(struct inode *inode,
 		return -EINVAL;
 	}
 
-	ctxsize = fscrypt_new_context_from_policy(&ctx, policy);
+	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
+	ctxsize = fscrypt_new_context(&ctx, policy, nonce);
 
 	return inode->i_sb->s_cop->set_context(inode, &ctx, ctxsize, NULL);
 }
@@ -637,6 +640,7 @@ EXPORT_SYMBOL(fscrypt_has_permitted_context);
 int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 						void *fs_data, bool preload)
 {
+	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
 	union fscrypt_context ctx;
 	int ctxsize;
 	struct fscrypt_info *ci;
@@ -650,7 +654,8 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 	if (ci == NULL)
 		return -ENOKEY;
 
-	ctxsize = fscrypt_new_context_from_policy(&ctx, &ci->ci_policy);
+	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
+	ctxsize = fscrypt_new_context(&ctx, &ci->ci_policy, nonce);
 
 	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
 	res = parent->i_sb->s_cop->set_context(child, &ctx, ctxsize, fs_data);
@@ -660,6 +665,45 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 }
 EXPORT_SYMBOL(fscrypt_inherit_context);
 
+/**
+ * fscrypt_set_context() - Set the fscrypt context of a new inode
+ * @inode: a new inode
+ * @fs_data: private data given by FS and passed to ->set_context()
+ *
+ * This should be called after fscrypt_prepare_new_inode(), generally during a
+ * filesystem transaction.  Everything here must be %GFP_NOFS-safe.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+int fscrypt_set_context(struct inode *inode, void *fs_data)
+{
+	struct fscrypt_info *ci = inode->i_crypt_info;
+	union fscrypt_context ctx;
+	int ctxsize;
+
+	/* fscrypt_prepare_new_inode() should have set up the key already. */
+	if (WARN_ON_ONCE(!ci))
+		return -ENOKEY;
+
+	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
+	ctxsize = fscrypt_new_context(&ctx, &ci->ci_policy, ci->ci_nonce);
+
+	/*
+	 * This may be the first time the inode number is available, so do any
+	 * delayed key setup that requires the inode number.
+	 */
+	if (ci->ci_policy.version == FSCRYPT_POLICY_V2 &&
+	    (ci->ci_policy.v2.flags & FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32)) {
+		const struct fscrypt_master_key *mk =
+			ci->ci_master_key->payload.data[0];
+
+		fscrypt_hash_inode_number(ci, mk);
+	}
+
+	return inode->i_sb->s_cop->set_context(inode, &ctx, ctxsize, fs_data);
+}
+EXPORT_SYMBOL_GPL(fscrypt_set_context);
+
 /**
  * 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 eaf16eb557887..9cf7ca90f3abb 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -158,6 +158,7 @@ int fscrypt_ioctl_get_nonce(struct file *filp, void __user *arg);
 int fscrypt_has_permitted_context(struct inode *parent, struct inode *child);
 int fscrypt_inherit_context(struct inode *parent, struct inode *child,
 			    void *fs_data, bool preload);
+int fscrypt_set_context(struct inode *inode, void *fs_data);
 
 struct fscrypt_dummy_context {
 	const union fscrypt_context *ctx;
@@ -184,6 +185,8 @@ int fscrypt_ioctl_get_key_status(struct file *filp, void __user *arg);
 
 /* keysetup.c */
 int fscrypt_get_encryption_info(struct inode *inode);
+int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
+			      bool *encrypt_ret);
 void fscrypt_put_encryption_info(struct inode *inode);
 void fscrypt_free_inode(struct inode *inode);
 int fscrypt_drop_inode(struct inode *inode);
@@ -347,6 +350,11 @@ static inline int fscrypt_inherit_context(struct inode *parent,
 	return -EOPNOTSUPP;
 }
 
+static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
+{
+	return -EOPNOTSUPP;
+}
+
 struct fscrypt_dummy_context {
 };
 
@@ -394,6 +402,15 @@ static inline int fscrypt_get_encryption_info(struct inode *inode)
 	return -EOPNOTSUPP;
 }
 
+static inline int fscrypt_prepare_new_inode(struct inode *dir,
+					    struct inode *inode,
+					    bool *encrypt_ret)
+{
+	if (IS_ENCRYPTED(dir))
+		return -EOPNOTSUPP;
+	return 0;
+}
+
 static inline void fscrypt_put_encryption_info(struct inode *inode)
 {
 	return;
-- 
2.28.0


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

* [PATCH v3 02/13] ext4: factor out ext4_xattr_credits_for_new_inode()
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 01/13] fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 03/13] ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

To compute a new inode's xattr credits, we need to know whether the
inode will be encrypted or not.  When we switch to use the new helper
function fscrypt_prepare_new_inode(), we won't find out whether the
inode will be encrypted until slightly later than is currently the case.
That will require moving the code block that computes the xattr credits.

To make this easier and reduce the length of __ext4_new_inode(), move
this code block into a new function ext4_xattr_credits_for_new_inode().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/ialloc.c | 90 +++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 39 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index df25d38d65393..0cc576005a923 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -742,6 +742,53 @@ static int find_inode_bit(struct super_block *sb, ext4_group_t group,
 	return 1;
 }
 
+static int ext4_xattr_credits_for_new_inode(struct inode *dir, mode_t mode,
+					    bool encrypt)
+{
+	struct super_block *sb = dir->i_sb;
+	int nblocks = 0;
+#ifdef CONFIG_EXT4_FS_POSIX_ACL
+	struct posix_acl *p = get_acl(dir, ACL_TYPE_DEFAULT);
+
+	if (IS_ERR(p))
+		return PTR_ERR(p);
+	if (p) {
+		int acl_size = p->a_count * sizeof(ext4_acl_entry);
+
+		nblocks += (S_ISDIR(mode) ? 2 : 1) *
+			__ext4_xattr_set_credits(sb, NULL /* inode */,
+						 NULL /* block_bh */, acl_size,
+						 true /* is_create */);
+		posix_acl_release(p);
+	}
+#endif
+
+#ifdef CONFIG_SECURITY
+	{
+		int num_security_xattrs = 1;
+
+#ifdef CONFIG_INTEGRITY
+		num_security_xattrs++;
+#endif
+		/*
+		 * We assume that security xattrs are never more than 1k.
+		 * In practice they are under 128 bytes.
+		 */
+		nblocks += num_security_xattrs *
+			__ext4_xattr_set_credits(sb, NULL /* inode */,
+						 NULL /* block_bh */, 1024,
+						 true /* is_create */);
+	}
+#endif
+	if (encrypt)
+		nblocks += __ext4_xattr_set_credits(sb,
+						    NULL /* inode */,
+						    NULL /* block_bh */,
+						    FSCRYPT_SET_CONTEXT_MAX_SIZE,
+						    true /* is_create */);
+	return nblocks;
+}
+
 /*
  * There are two policies for allocating an inode.  If the new inode is
  * a directory, then a forward search is made for a block group with both
@@ -796,45 +843,10 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	}
 
 	if (!handle && sbi->s_journal && !(i_flags & EXT4_EA_INODE_FL)) {
-#ifdef CONFIG_EXT4_FS_POSIX_ACL
-		struct posix_acl *p = get_acl(dir, ACL_TYPE_DEFAULT);
-
-		if (IS_ERR(p))
-			return ERR_CAST(p);
-		if (p) {
-			int acl_size = p->a_count * sizeof(ext4_acl_entry);
-
-			nblocks += (S_ISDIR(mode) ? 2 : 1) *
-				__ext4_xattr_set_credits(sb, NULL /* inode */,
-					NULL /* block_bh */, acl_size,
-					true /* is_create */);
-			posix_acl_release(p);
-		}
-#endif
-
-#ifdef CONFIG_SECURITY
-		{
-			int num_security_xattrs = 1;
-
-#ifdef CONFIG_INTEGRITY
-			num_security_xattrs++;
-#endif
-			/*
-			 * We assume that security xattrs are never
-			 * more than 1k.  In practice they are under
-			 * 128 bytes.
-			 */
-			nblocks += num_security_xattrs *
-				__ext4_xattr_set_credits(sb, NULL /* inode */,
-					NULL /* block_bh */, 1024,
-					true /* is_create */);
-		}
-#endif
-		if (encrypt)
-			nblocks += __ext4_xattr_set_credits(sb,
-					NULL /* inode */, NULL /* block_bh */,
-					FSCRYPT_SET_CONTEXT_MAX_SIZE,
-					true /* is_create */);
+		ret2 = ext4_xattr_credits_for_new_inode(dir, mode, encrypt);
+		if (ret2 < 0)
+			return ERR_PTR(ret2);
+		nblocks += ret2;
 	}
 
 	ngroups = ext4_get_groups_count(sb);
-- 
2.28.0


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

* [PATCH v3 03/13] ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context()
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 01/13] fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 02/13] ext4: factor out ext4_xattr_credits_for_new_inode() Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 04/13] f2fs: " Eric Biggers
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

Convert ext4 to use the new functions fscrypt_prepare_new_inode() and
fscrypt_set_context().  This avoids calling
fscrypt_get_encryption_info() from within a transaction, which can
deadlock because fscrypt_get_encryption_info() isn't GFP_NOFS-safe.

For more details about this problem, see the earlier patch
"fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()".

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/ialloc.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 0cc576005a923..698ca4a4db5f7 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -819,7 +819,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	ext4_group_t i;
 	ext4_group_t flex_group;
 	struct ext4_group_info *grp;
-	int encrypt = 0;
+	bool encrypt = false;
 
 	/* Cannot create files in a deleted directory */
 	if (!dir || !dir->i_nlink)
@@ -831,24 +831,6 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	if (unlikely(ext4_forced_shutdown(sbi)))
 		return ERR_PTR(-EIO);
 
-	if ((IS_ENCRYPTED(dir) || DUMMY_ENCRYPTION_ENABLED(sbi)) &&
-	    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)) &&
-	    !(i_flags & EXT4_EA_INODE_FL)) {
-		err = fscrypt_get_encryption_info(dir);
-		if (err)
-			return ERR_PTR(err);
-		if (!fscrypt_has_encryption_key(dir))
-			return ERR_PTR(-ENOKEY);
-		encrypt = 1;
-	}
-
-	if (!handle && sbi->s_journal && !(i_flags & EXT4_EA_INODE_FL)) {
-		ret2 = ext4_xattr_credits_for_new_inode(dir, mode, encrypt);
-		if (ret2 < 0)
-			return ERR_PTR(ret2);
-		nblocks += ret2;
-	}
-
 	ngroups = ext4_get_groups_count(sb);
 	trace_ext4_request_inode(dir, mode);
 	inode = new_inode(sb);
@@ -878,10 +860,25 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	else
 		ei->i_projid = make_kprojid(&init_user_ns, EXT4_DEF_PROJID);
 
+	if (!(i_flags & EXT4_EA_INODE_FL)) {
+		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
+		if (err)
+			goto out;
+	}
+
 	err = dquot_initialize(inode);
 	if (err)
 		goto out;
 
+	if (!handle && sbi->s_journal && !(i_flags & EXT4_EA_INODE_FL)) {
+		ret2 = ext4_xattr_credits_for_new_inode(dir, mode, encrypt);
+		if (ret2 < 0) {
+			err = ret2;
+			goto out;
+		}
+		nblocks += ret2;
+	}
+
 	if (!goal)
 		goal = sbi->s_inode_goal;
 
@@ -1174,7 +1171,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 	 * prevent its deduplication.
 	 */
 	if (encrypt) {
-		err = fscrypt_inherit_context(dir, inode, handle, true);
+		err = fscrypt_set_context(inode, handle);
 		if (err)
 			goto fail_free_drop;
 	}
-- 
2.28.0


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

* [PATCH v3 04/13] f2fs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (2 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 03/13] ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 05/13] ubifs: " Eric Biggers
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg, Jaegeuk Kim

From: Eric Biggers <ebiggers@google.com>

Convert f2fs to use the new functions fscrypt_prepare_new_inode() and
fscrypt_set_context().  This avoids calling
fscrypt_get_encryption_info() from under f2fs_lock_op(), which can
deadlock because fscrypt_get_encryption_info() isn't GFP_NOFS-safe.

For more details about this problem, see the earlier patch
"fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()".

This also fixes a f2fs-specific deadlock when the filesystem is mounted
with '-o test_dummy_encryption' and a file is created in an unencrypted
directory other than the root directory:

    INFO: task touch:207 blocked for more than 30 seconds.
          Not tainted 5.9.0-rc4-00099-g729e3d0919844 #2
    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
    task:touch           state:D stack:    0 pid:  207 ppid:   167 flags:0x00000000
    Call Trace:
     [...]
     lock_page include/linux/pagemap.h:548 [inline]
     pagecache_get_page+0x25e/0x310 mm/filemap.c:1682
     find_or_create_page include/linux/pagemap.h:348 [inline]
     grab_cache_page include/linux/pagemap.h:424 [inline]
     f2fs_grab_cache_page fs/f2fs/f2fs.h:2395 [inline]
     f2fs_grab_cache_page fs/f2fs/f2fs.h:2373 [inline]
     __get_node_page.part.0+0x39/0x2d0 fs/f2fs/node.c:1350
     __get_node_page fs/f2fs/node.c:35 [inline]
     f2fs_get_node_page+0x2e/0x60 fs/f2fs/node.c:1399
     read_inline_xattr+0x88/0x140 fs/f2fs/xattr.c:288
     lookup_all_xattrs+0x1f9/0x2c0 fs/f2fs/xattr.c:344
     f2fs_getxattr+0x9b/0x160 fs/f2fs/xattr.c:532
     f2fs_get_context+0x1e/0x20 fs/f2fs/super.c:2460
     fscrypt_get_encryption_info+0x9b/0x450 fs/crypto/keysetup.c:472
     fscrypt_inherit_context+0x2f/0xb0 fs/crypto/policy.c:640
     f2fs_init_inode_metadata+0xab/0x340 fs/f2fs/dir.c:540
     f2fs_add_inline_entry+0x145/0x390 fs/f2fs/inline.c:621
     f2fs_add_dentry+0x31/0x80 fs/f2fs/dir.c:757
     f2fs_do_add_link+0xcd/0x130 fs/f2fs/dir.c:798
     f2fs_add_link fs/f2fs/f2fs.h:3234 [inline]
     f2fs_create+0x104/0x290 fs/f2fs/namei.c:344
     lookup_open.isra.0+0x2de/0x500 fs/namei.c:3103
     open_last_lookups+0xa9/0x340 fs/namei.c:3177
     path_openat+0x8f/0x1b0 fs/namei.c:3365
     do_filp_open+0x87/0x130 fs/namei.c:3395
     do_sys_openat2+0x96/0x150 fs/open.c:1168
     [...]

That happened because f2fs_add_inline_entry() locks the directory
inode's page in order to add the dentry, then f2fs_get_context() tries
to lock it recursively in order to read the encryption xattr.  This
problem is specific to "test_dummy_encryption" because normally the
directory's fscrypt_info would be set up prior to
f2fs_add_inline_entry() in order to encrypt the new filename.

Regardless, the new design fixes this test_dummy_encryption deadlock as
well as potential deadlocks with fs reclaim, by setting up any needed
fscrypt_info structs prior to taking so many locks.

The test_dummy_encryption deadlock was reported by Daniel Rosenberg.

Reported-by: Daniel Rosenberg <drosen@google.com>
Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/dir.c   |  2 +-
 fs/f2fs/f2fs.h  | 23 -----------------------
 fs/f2fs/namei.c |  7 ++++++-
 3 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index b2530b9507bd9..414bc94fbd546 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -537,7 +537,7 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir,
 			goto put_error;
 
 		if (IS_ENCRYPTED(inode)) {
-			err = fscrypt_inherit_context(dir, inode, page, false);
+			err = fscrypt_set_context(inode, page);
 			if (err)
 				goto put_error;
 		}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d9e52a7f3702f..0503371f88ed4 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1315,13 +1315,6 @@ enum fsync_mode {
 #define IS_IO_TRACED_PAGE(page) (0)
 #endif
 
-#ifdef CONFIG_FS_ENCRYPTION
-#define DUMMY_ENCRYPTION_ENABLED(sbi) \
-	(unlikely(F2FS_OPTION(sbi).dummy_enc_ctx.ctx != NULL))
-#else
-#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
-#endif
-
 /* For compression */
 enum compress_algorithm_type {
 	COMPRESS_LZO,
@@ -4022,22 +4015,6 @@ static inline bool f2fs_lfs_mode(struct f2fs_sb_info *sbi)
 	return F2FS_OPTION(sbi).fs_mode == FS_MODE_LFS;
 }
 
-static inline bool f2fs_may_encrypt(struct inode *dir, struct inode *inode)
-{
-#ifdef CONFIG_FS_ENCRYPTION
-	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
-	umode_t mode = inode->i_mode;
-
-	/*
-	 * If the directory encrypted or dummy encryption enabled,
-	 * then we should encrypt the inode.
-	 */
-	if (IS_ENCRYPTED(dir) || DUMMY_ENCRYPTION_ENABLED(sbi))
-		return (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode));
-#endif
-	return false;
-}
-
 static inline bool f2fs_may_compress(struct inode *inode)
 {
 	if (IS_SWAPFILE(inode) || f2fs_is_pinned_file(inode) ||
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 84e4bbc1a64de..45f324511a19e 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -28,6 +28,7 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
 	nid_t ino;
 	struct inode *inode;
 	bool nid_free = false;
+	bool encrypt = false;
 	int xattr_size = 0;
 	int err;
 
@@ -69,13 +70,17 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
 		F2FS_I(inode)->i_projid = make_kprojid(&init_user_ns,
 							F2FS_DEF_PROJID);
 
+	err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
+	if (err)
+		goto fail_drop;
+
 	err = dquot_initialize(inode);
 	if (err)
 		goto fail_drop;
 
 	set_inode_flag(inode, FI_NEW_INODE);
 
-	if (f2fs_may_encrypt(dir, inode))
+	if (encrypt)
 		f2fs_set_encrypted_inode(inode);
 
 	if (f2fs_sb_has_extra_attr(sbi)) {
-- 
2.28.0


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

* [PATCH v3 05/13] ubifs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (3 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 04/13] f2fs: " Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 06/13] fscrypt: adjust logging for in-creation inodes Eric Biggers
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

Convert ubifs to use the new functions fscrypt_prepare_new_inode() and
fscrypt_set_context().

Unlike ext4 and f2fs, this doesn't appear to fix any deadlock bug.  But
it does shorten the code slightly and get all filesystems using the same
helper functions, so that fscrypt_inherit_context() can be removed.

It also fixes an incorrect error code where ubifs returned EPERM instead
of the expected ENOKEY.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ubifs/dir.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index a9c1f5a9c9bdd..155521e51ac57 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -81,19 +81,6 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 	struct ubifs_inode *ui;
 	bool encrypted = false;
 
-	if (IS_ENCRYPTED(dir)) {
-		err = fscrypt_get_encryption_info(dir);
-		if (err) {
-			ubifs_err(c, "fscrypt_get_encryption_info failed: %i", err);
-			return ERR_PTR(err);
-		}
-
-		if (!fscrypt_has_encryption_key(dir))
-			return ERR_PTR(-EPERM);
-
-		encrypted = true;
-	}
-
 	inode = new_inode(c->vfs_sb);
 	ui = ubifs_inode(inode);
 	if (!inode)
@@ -112,6 +99,12 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 			 current_time(inode);
 	inode->i_mapping->nrpages = 0;
 
+	err = fscrypt_prepare_new_inode(dir, inode, &encrypted);
+	if (err) {
+		ubifs_err(c, "fscrypt_prepare_new_inode failed: %i", err);
+		goto out_iput;
+	}
+
 	switch (mode & S_IFMT) {
 	case S_IFREG:
 		inode->i_mapping->a_ops = &ubifs_file_address_operations;
@@ -131,7 +124,6 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 	case S_IFBLK:
 	case S_IFCHR:
 		inode->i_op  = &ubifs_file_inode_operations;
-		encrypted = false;
 		break;
 	default:
 		BUG();
@@ -151,9 +143,8 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 		if (c->highest_inum >= INUM_WATERMARK) {
 			spin_unlock(&c->cnt_lock);
 			ubifs_err(c, "out of inode numbers");
-			make_bad_inode(inode);
-			iput(inode);
-			return ERR_PTR(-EINVAL);
+			err = -EINVAL;
+			goto out_iput;
 		}
 		ubifs_warn(c, "running out of inode numbers (current %lu, max %u)",
 			   (unsigned long)c->highest_inum, INUM_WATERMARK);
@@ -171,16 +162,19 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 	spin_unlock(&c->cnt_lock);
 
 	if (encrypted) {
-		err = fscrypt_inherit_context(dir, inode, &encrypted, true);
+		err = fscrypt_set_context(inode, NULL);
 		if (err) {
-			ubifs_err(c, "fscrypt_inherit_context failed: %i", err);
-			make_bad_inode(inode);
-			iput(inode);
-			return ERR_PTR(err);
+			ubifs_err(c, "fscrypt_set_context failed: %i", err);
+			goto out_iput;
 		}
 	}
 
 	return inode;
+
+out_iput:
+	make_bad_inode(inode);
+	iput(inode);
+	return ERR_PTR(err);
 }
 
 static int dbg_check_name(const struct ubifs_info *c,
-- 
2.28.0


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

* [PATCH v3 06/13] fscrypt: adjust logging for in-creation inodes
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (4 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 05/13] ubifs: " Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 07/13] fscrypt: remove fscrypt_inherit_context() Eric Biggers
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

Now that a fscrypt_info may be set up for inodes that are currently
being created and haven't yet had an inode number assigned, avoid
logging confusing messages about "inode 0".

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/crypto.c  | 4 +++-
 fs/crypto/keyring.c | 9 +++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 9212325763b0f..4ef3f714046aa 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -343,9 +343,11 @@ void fscrypt_msg(const struct inode *inode, const char *level,
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
-	if (inode)
+	if (inode && inode->i_ino)
 		printk("%sfscrypt (%s, inode %lu): %pV\n",
 		       level, inode->i_sb->s_id, inode->i_ino, &vaf);
+	else if (inode)
+		printk("%sfscrypt (%s): %pV\n", level, inode->i_sb->s_id, &vaf);
 	else
 		printk("%sfscrypt: %pV\n", level, &vaf);
 	va_end(args);
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index e74f239c44280..53cc552a7b8fd 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -817,6 +817,7 @@ static int check_for_busy_inodes(struct super_block *sb,
 	struct list_head *pos;
 	size_t busy_count = 0;
 	unsigned long ino;
+	char ino_str[50] = "";
 
 	spin_lock(&mk->mk_decrypted_inodes_lock);
 
@@ -838,11 +839,15 @@ static int check_for_busy_inodes(struct super_block *sb,
 	}
 	spin_unlock(&mk->mk_decrypted_inodes_lock);
 
+	/* If the inode is currently being created, ino may still be 0. */
+	if (ino)
+		snprintf(ino_str, sizeof(ino_str), ", including ino %lu", ino);
+
 	fscrypt_warn(NULL,
-		     "%s: %zu inode(s) still busy after removing key with %s %*phN, including ino %lu",
+		     "%s: %zu inode(s) still busy after removing key with %s %*phN%s",
 		     sb->s_id, busy_count, master_key_spec_type(&mk->mk_spec),
 		     master_key_spec_len(&mk->mk_spec), (u8 *)&mk->mk_spec.u,
-		     ino);
+		     ino_str);
 	return -EBUSY;
 }
 
-- 
2.28.0


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

* [PATCH v3 07/13] fscrypt: remove fscrypt_inherit_context()
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (5 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 06/13] fscrypt: adjust logging for in-creation inodes Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 08/13] fscrypt: require that fscrypt_encrypt_symlink() already has key Eric Biggers
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

Now that all filesystems have been converted to use
fscrypt_prepare_new_inode() and fscrypt_set_context(),
fscrypt_inherit_context() is no longer used.  Remove it.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c      | 37 -------------------------------------
 include/linux/fscrypt.h |  9 ---------
 2 files changed, 46 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 7e96953d385ec..4ff893f7b030a 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -628,43 +628,6 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 }
 EXPORT_SYMBOL(fscrypt_has_permitted_context);
 
-/**
- * fscrypt_inherit_context() - Sets a child context from its parent
- * @parent: Parent inode from which the context is inherited.
- * @child:  Child inode that inherits the context from @parent.
- * @fs_data:  private data given by FS.
- * @preload:  preload child i_crypt_info if true
- *
- * Return: 0 on success, -errno on failure
- */
-int fscrypt_inherit_context(struct inode *parent, struct inode *child,
-						void *fs_data, bool preload)
-{
-	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
-	union fscrypt_context ctx;
-	int ctxsize;
-	struct fscrypt_info *ci;
-	int res;
-
-	res = fscrypt_get_encryption_info(parent);
-	if (res < 0)
-		return res;
-
-	ci = fscrypt_get_info(parent);
-	if (ci == NULL)
-		return -ENOKEY;
-
-	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
-	ctxsize = fscrypt_new_context(&ctx, &ci->ci_policy, nonce);
-
-	BUILD_BUG_ON(sizeof(ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE);
-	res = parent->i_sb->s_cop->set_context(child, &ctx, ctxsize, fs_data);
-	if (res)
-		return res;
-	return preload ? fscrypt_get_encryption_info(child): 0;
-}
-EXPORT_SYMBOL(fscrypt_inherit_context);
-
 /**
  * fscrypt_set_context() - Set the fscrypt context of a new inode
  * @inode: a new inode
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 9cf7ca90f3abb..81d6ded243288 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -156,8 +156,6 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg);
 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_inherit_context(struct inode *parent, struct inode *child,
-			    void *fs_data, bool preload);
 int fscrypt_set_context(struct inode *inode, void *fs_data);
 
 struct fscrypt_dummy_context {
@@ -343,13 +341,6 @@ static inline int fscrypt_has_permitted_context(struct inode *parent,
 	return 0;
 }
 
-static inline int fscrypt_inherit_context(struct inode *parent,
-					  struct inode *child,
-					  void *fs_data, bool preload)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
 {
 	return -EOPNOTSUPP;
-- 
2.28.0


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

* [PATCH v3 08/13] fscrypt: require that fscrypt_encrypt_symlink() already has key
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (6 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 07/13] fscrypt: remove fscrypt_inherit_context() Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 09/13] fscrypt: stop pretending that key setup is nofs-safe Eric Biggers
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

Now that all filesystems have been converted to use
fscrypt_prepare_new_inode(), the encryption key for new symlink inodes
is now already set up whenever we try to encrypt the symlink target.
Enforce this rather than try to set up the key again when it may be too
late to do so safely.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/hooks.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 491b252843eb9..7748db5092409 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -217,9 +217,13 @@ int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
 	struct fscrypt_symlink_data *sd;
 	unsigned int ciphertext_len;
 
-	err = fscrypt_require_key(inode);
-	if (err)
-		return err;
+	/*
+	 * fscrypt_prepare_new_inode() should have already set up the new
+	 * symlink inode's encryption key.  We don't wait until now to do it,
+	 * since we may be in a filesystem transaction now.
+	 */
+	if (WARN_ON_ONCE(!fscrypt_has_encryption_key(inode)))
+		return -ENOKEY;
 
 	if (disk_link->name) {
 		/* filesystem-provided buffer */
-- 
2.28.0


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

* [PATCH v3 09/13] fscrypt: stop pretending that key setup is nofs-safe
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (7 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 08/13] fscrypt: require that fscrypt_encrypt_symlink() already has key Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 10/13] fscrypt: make "#define fscrypt_policy" user-only Eric Biggers
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

fscrypt_get_encryption_info() has never actually been safe to call in a
context that needs GFP_NOFS, since it calls crypto_alloc_skcipher().

crypto_alloc_skcipher() isn't GFP_NOFS-safe, even if called under
memalloc_nofs_save().  This is because it may load kernel modules, and
also because it internally takes crypto_alg_sem.  Other tasks can do
GFP_KERNEL allocations while holding crypto_alg_sem for write.

The use of fscrypt_init_mutex isn't GFP_NOFS-safe either.

So, stop pretending that fscrypt_get_encryption_info() is nofs-safe.
I.e., when it allocates memory, just use GFP_KERNEL instead of GFP_NOFS.

Note, another reason to do this is that GFP_NOFS is deprecated in favor
of using memalloc_nofs_save() in the proper places.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/inline_crypt.c | 7 ++-----
 fs/crypto/keysetup.c     | 2 +-
 fs/crypto/keysetup_v1.c  | 8 ++++----
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index faa25541ccb68..89bffa82ed74a 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -106,7 +106,7 @@ int fscrypt_select_encryption_impl(struct fscrypt_info *ci)
 	crypto_cfg.data_unit_size = sb->s_blocksize;
 	crypto_cfg.dun_bytes = fscrypt_get_dun_bytes(ci);
 	num_devs = fscrypt_get_num_devices(sb);
-	devs = kmalloc_array(num_devs, sizeof(*devs), GFP_NOFS);
+	devs = kmalloc_array(num_devs, sizeof(*devs), GFP_KERNEL);
 	if (!devs)
 		return -ENOMEM;
 	fscrypt_get_devices(sb, num_devs, devs);
@@ -135,9 +135,8 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 	struct fscrypt_blk_crypto_key *blk_key;
 	int err;
 	int i;
-	unsigned int flags;
 
-	blk_key = kzalloc(struct_size(blk_key, devs, num_devs), GFP_NOFS);
+	blk_key = kzalloc(struct_size(blk_key, devs, num_devs), GFP_KERNEL);
 	if (!blk_key)
 		return -ENOMEM;
 
@@ -166,10 +165,8 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 		}
 		queue_refs++;
 
-		flags = memalloc_nofs_save();
 		err = blk_crypto_start_using_key(&blk_key->base,
 						 blk_key->devs[i]);
-		memalloc_nofs_restore(flags);
 		if (err) {
 			fscrypt_err(inode,
 				    "error %d starting to use blk-crypto", err);
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 6159168972146..47f19061ba10e 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -488,7 +488,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	if (res)
 		return res;
 
-	crypt_info = kmem_cache_zalloc(fscrypt_info_cachep, GFP_NOFS);
+	crypt_info = kmem_cache_zalloc(fscrypt_info_cachep, GFP_KERNEL);
 	if (!crypt_info)
 		return -ENOMEM;
 
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index a3cb52572b05c..2762c53504323 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -60,7 +60,7 @@ static int derive_key_aes(const u8 *master_key,
 		goto out;
 	}
 	crypto_skcipher_set_flags(tfm, CRYPTO_TFM_REQ_FORBID_WEAK_KEYS);
-	req = skcipher_request_alloc(tfm, GFP_NOFS);
+	req = skcipher_request_alloc(tfm, GFP_KERNEL);
 	if (!req) {
 		res = -ENOMEM;
 		goto out;
@@ -99,7 +99,7 @@ find_and_lock_process_key(const char *prefix,
 	const struct user_key_payload *ukp;
 	const struct fscrypt_key *payload;
 
-	description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
+	description = kasprintf(GFP_KERNEL, "%s%*phN", prefix,
 				FSCRYPT_KEY_DESCRIPTOR_SIZE, descriptor);
 	if (!description)
 		return ERR_PTR(-ENOMEM);
@@ -228,7 +228,7 @@ fscrypt_get_direct_key(const struct fscrypt_info *ci, const u8 *raw_key)
 		return dk;
 
 	/* Nope, allocate one. */
-	dk = kzalloc(sizeof(*dk), GFP_NOFS);
+	dk = kzalloc(sizeof(*dk), GFP_KERNEL);
 	if (!dk)
 		return ERR_PTR(-ENOMEM);
 	refcount_set(&dk->dk_refcount, 1);
@@ -272,7 +272,7 @@ static int setup_v1_file_key_derived(struct fscrypt_info *ci,
 	 * This cannot be a stack buffer because it will be passed to the
 	 * scatterlist crypto API during derive_key_aes().
 	 */
-	derived_key = kmalloc(ci->ci_mode->keysize, GFP_NOFS);
+	derived_key = kmalloc(ci->ci_mode->keysize, GFP_KERNEL);
 	if (!derived_key)
 		return -ENOMEM;
 
-- 
2.28.0


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

* [PATCH v3 10/13] fscrypt: make "#define fscrypt_policy" user-only
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (8 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 09/13] fscrypt: stop pretending that key setup is nofs-safe Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 11/13] fscrypt: move fscrypt_prepare_symlink() out-of-line Eric Biggers
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

The fscrypt UAPI header defines fscrypt_policy to fscrypt_policy_v1,
for source compatibility with old userspace programs.

Internally, the kernel doesn't want that compatibility definition.
Instead, fscrypt_private.h #undefs it and re-defines it to a union.

That works for now.  However, in order to add
fscrypt_operations::get_dummy_policy(), we'll need to forward declare
'union fscrypt_policy' in include/linux/fscrypt.h.  That would cause
build errors because "fscrypt_policy" is used in ioctl numbers.

To avoid this, modify the UAPI header to make the fscrypt_policy
compatibility definition conditional on !__KERNEL__, and make the ioctls
use fscrypt_policy_v1 instead of fscrypt_policy.

Note that this doesn't change the actual ioctl numbers.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h  | 1 -
 include/uapi/linux/fscrypt.h | 6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 355f6d9377517..ac3352086ee44 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -97,7 +97,6 @@ static inline const u8 *fscrypt_context_nonce(const union fscrypt_context *ctx)
 	return NULL;
 }
 
-#undef fscrypt_policy
 union fscrypt_policy {
 	u8 version;
 	struct fscrypt_policy_v1 v1;
diff --git a/include/uapi/linux/fscrypt.h b/include/uapi/linux/fscrypt.h
index 7875709ccfebf..e5de603369381 100644
--- a/include/uapi/linux/fscrypt.h
+++ b/include/uapi/linux/fscrypt.h
@@ -45,7 +45,6 @@ struct fscrypt_policy_v1 {
 	__u8 flags;
 	__u8 master_key_descriptor[FSCRYPT_KEY_DESCRIPTOR_SIZE];
 };
-#define fscrypt_policy	fscrypt_policy_v1
 
 /*
  * Process-subscribed "logon" key description prefix and payload format.
@@ -156,9 +155,9 @@ struct fscrypt_get_key_status_arg {
 	__u32 __out_reserved[13];
 };
 
-#define FS_IOC_SET_ENCRYPTION_POLICY		_IOR('f', 19, struct fscrypt_policy)
+#define FS_IOC_SET_ENCRYPTION_POLICY		_IOR('f', 19, struct fscrypt_policy_v1)
 #define FS_IOC_GET_ENCRYPTION_PWSALT		_IOW('f', 20, __u8[16])
-#define FS_IOC_GET_ENCRYPTION_POLICY		_IOW('f', 21, struct fscrypt_policy)
+#define FS_IOC_GET_ENCRYPTION_POLICY		_IOW('f', 21, struct fscrypt_policy_v1)
 #define FS_IOC_GET_ENCRYPTION_POLICY_EX		_IOWR('f', 22, __u8[9]) /* size + version */
 #define FS_IOC_ADD_ENCRYPTION_KEY		_IOWR('f', 23, struct fscrypt_add_key_arg)
 #define FS_IOC_REMOVE_ENCRYPTION_KEY		_IOWR('f', 24, struct fscrypt_remove_key_arg)
@@ -170,6 +169,7 @@ struct fscrypt_get_key_status_arg {
 
 /* old names; don't add anything new here! */
 #ifndef __KERNEL__
+#define fscrypt_policy			fscrypt_policy_v1
 #define FS_KEY_DESCRIPTOR_SIZE		FSCRYPT_KEY_DESCRIPTOR_SIZE
 #define FS_POLICY_FLAGS_PAD_4		FSCRYPT_POLICY_FLAGS_PAD_4
 #define FS_POLICY_FLAGS_PAD_8		FSCRYPT_POLICY_FLAGS_PAD_8
-- 
2.28.0


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

* [PATCH v3 11/13] fscrypt: move fscrypt_prepare_symlink() out-of-line
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (9 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 10/13] fscrypt: make "#define fscrypt_policy" user-only Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 12/13] fscrypt: handle test_dummy_encryption in more logical way Eric Biggers
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

In preparation for moving the logic for "get the encryption policy
inherited by new files in this directory" to a single place, make
fscrypt_prepare_symlink() a regular function rather than an inline
function that wraps __fscrypt_prepare_symlink().

This way, the new function fscrypt_policy_to_inherit() won't need to be
exported to filesystems.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/hooks.c       | 39 ++++++++++++++++++++++---
 include/linux/fscrypt.h | 63 ++++++++++-------------------------------
 2 files changed, 50 insertions(+), 52 deletions(-)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 7748db5092409..a399c54947f28 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -166,12 +166,43 @@ int fscrypt_prepare_setflags(struct inode *inode,
 	return 0;
 }
 
-int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
-			      unsigned int max_len,
-			      struct fscrypt_str *disk_link)
+/**
+ * fscrypt_prepare_symlink() - prepare to create a possibly-encrypted symlink
+ * @dir: directory in which the symlink is being created
+ * @target: plaintext symlink target
+ * @len: length of @target excluding null terminator
+ * @max_len: space the filesystem has available to store the symlink target
+ * @disk_link: (out) the on-disk symlink target being prepared
+ *
+ * This function computes the size the symlink target will require on-disk,
+ * stores it in @disk_link->len, and validates it against @max_len.  An
+ * encrypted symlink may be longer than the original.
+ *
+ * Additionally, @disk_link->name is set to @target if the symlink will be
+ * unencrypted, but left NULL if the symlink will be encrypted.  For encrypted
+ * symlinks, the filesystem must call fscrypt_encrypt_symlink() to create the
+ * on-disk target later.  (The reason for the two-step process is that some
+ * filesystems need to know the size of the symlink target before creating the
+ * inode, e.g. to determine whether it will be a "fast" or "slow" symlink.)
+ *
+ * Return: 0 on success, -ENAMETOOLONG if the symlink target is too long,
+ * -ENOKEY if the encryption key is missing, or another -errno code if a problem
+ * occurred while setting up the encryption key.
+ */
+int fscrypt_prepare_symlink(struct inode *dir, const char *target,
+			    unsigned int len, unsigned int max_len,
+			    struct fscrypt_str *disk_link)
 {
 	int err;
 
+	if (!IS_ENCRYPTED(dir) && !fscrypt_get_dummy_context(dir->i_sb)) {
+		disk_link->name = (unsigned char *)target;
+		disk_link->len = len + 1;
+		if (disk_link->len > max_len)
+			return -ENAMETOOLONG;
+		return 0;
+	}
+
 	/*
 	 * To calculate the size of the encrypted symlink target we need to know
 	 * the amount of NUL padding, which is determined by the flags set in
@@ -207,7 +238,7 @@ int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
 	disk_link->name = NULL;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(__fscrypt_prepare_symlink);
+EXPORT_SYMBOL_GPL(fscrypt_prepare_symlink);
 
 int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
 			      unsigned int len, struct fscrypt_str *disk_link)
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 81d6ded243288..39e7397a3f103 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -225,9 +225,9 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry,
 			     struct fscrypt_name *fname);
 int fscrypt_prepare_setflags(struct inode *inode,
 			     unsigned int oldflags, unsigned int flags);
-int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
-			      unsigned int max_len,
-			      struct fscrypt_str *disk_link);
+int fscrypt_prepare_symlink(struct inode *dir, const char *target,
+			    unsigned int len, unsigned int max_len,
+			    struct fscrypt_str *disk_link);
 int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
 			      unsigned int len, struct fscrypt_str *disk_link);
 const char *fscrypt_get_symlink(struct inode *inode, const void *caddr,
@@ -520,15 +520,21 @@ static inline int fscrypt_prepare_setflags(struct inode *inode,
 	return 0;
 }
 
-static inline int __fscrypt_prepare_symlink(struct inode *dir,
-					    unsigned int len,
-					    unsigned int max_len,
-					    struct fscrypt_str *disk_link)
+static inline int fscrypt_prepare_symlink(struct inode *dir,
+					  const char *target,
+					  unsigned int len,
+					  unsigned int max_len,
+					  struct fscrypt_str *disk_link)
 {
-	return -EOPNOTSUPP;
+	if (IS_ENCRYPTED(dir))
+		return -EOPNOTSUPP;
+	disk_link->name = (unsigned char *)target;
+	disk_link->len = len + 1;
+	if (disk_link->len > max_len)
+		return -ENAMETOOLONG;
+	return 0;
 }
 
-
 static inline int __fscrypt_encrypt_symlink(struct inode *inode,
 					    const char *target,
 					    unsigned int len,
@@ -793,45 +799,6 @@ static inline int fscrypt_prepare_setattr(struct dentry *dentry,
 	return 0;
 }
 
-/**
- * fscrypt_prepare_symlink() - prepare to create a possibly-encrypted symlink
- * @dir: directory in which the symlink is being created
- * @target: plaintext symlink target
- * @len: length of @target excluding null terminator
- * @max_len: space the filesystem has available to store the symlink target
- * @disk_link: (out) the on-disk symlink target being prepared
- *
- * This function computes the size the symlink target will require on-disk,
- * stores it in @disk_link->len, and validates it against @max_len.  An
- * encrypted symlink may be longer than the original.
- *
- * Additionally, @disk_link->name is set to @target if the symlink will be
- * unencrypted, but left NULL if the symlink will be encrypted.  For encrypted
- * symlinks, the filesystem must call fscrypt_encrypt_symlink() to create the
- * on-disk target later.  (The reason for the two-step process is that some
- * filesystems need to know the size of the symlink target before creating the
- * inode, e.g. to determine whether it will be a "fast" or "slow" symlink.)
- *
- * Return: 0 on success, -ENAMETOOLONG if the symlink target is too long,
- * -ENOKEY if the encryption key is missing, or another -errno code if a problem
- * occurred while setting up the encryption key.
- */
-static inline int fscrypt_prepare_symlink(struct inode *dir,
-					  const char *target,
-					  unsigned int len,
-					  unsigned int max_len,
-					  struct fscrypt_str *disk_link)
-{
-	if (IS_ENCRYPTED(dir) || fscrypt_get_dummy_context(dir->i_sb) != NULL)
-		return __fscrypt_prepare_symlink(dir, len, max_len, disk_link);
-
-	disk_link->name = (unsigned char *)target;
-	disk_link->len = len + 1;
-	if (disk_link->len > max_len)
-		return -ENAMETOOLONG;
-	return 0;
-}
-
 /**
  * fscrypt_encrypt_symlink() - encrypt the symlink target if needed
  * @inode: symlink inode
-- 
2.28.0


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

* [PATCH v3 12/13] fscrypt: handle test_dummy_encryption in more logical way
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (10 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 11/13] fscrypt: move fscrypt_prepare_symlink() out-of-line Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17  4:11 ` [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *' Eric Biggers
  2020-09-21 22:35 ` [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
  13 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg, Jaegeuk Kim

From: Eric Biggers <ebiggers@google.com>

The behavior of the test_dummy_encryption mount option is that when a
new file (or directory or symlink) is created in an unencrypted
directory, it's automatically encrypted using a dummy encryption policy.
That's it; in particular, the encryption (or lack thereof) of existing
files (or directories or symlinks) doesn't change.

Unfortunately the implementation of test_dummy_encryption is a bit weird
and confusing.  When test_dummy_encryption is enabled and a file is
being created in an unencrypted directory, we set up an encryption key
(->i_crypt_info) for the directory.  This isn't actually used to do any
encryption, however, since the directory is still unencrypted!  Instead,
->i_crypt_info is only used for inheriting the encryption policy.

One consequence of this is that the filesystem ends up providing a
"dummy context" (policy + nonce) instead of a "dummy policy".  In
commit ed318a6cc0b6 ("fscrypt: support test_dummy_encryption=v2"), I
mistakenly thought this was required.  However, actually the nonce only
ends up being used to derive a key that is never used.

Another consequence of this implementation is that it allows for
'inode->i_crypt_info != NULL && !IS_ENCRYPTED(inode)', which is an edge
case that can be forgotten about.  For example, currently
FS_IOC_GET_ENCRYPTION_POLICY on an unencrypted directory may return the
dummy encryption policy when the filesystem is mounted with
test_dummy_encryption.  That seems like the wrong thing to do, since
again, the directory itself is not actually encrypted.

Therefore, switch to a more logical and maintainable implementation
where the dummy encryption policy inheritance is done without setting up
keys for unencrypted directories.  This involves:

- Adding a function fscrypt_policy_to_inherit() which returns the
  encryption policy to inherit from a directory.  This can be a real
  policy, a dummy policy, or no policy.

- Replacing struct fscrypt_dummy_context, ->get_dummy_context(), etc.
  with struct fscrypt_dummy_policy, ->get_dummy_policy(), etc.

- Making fscrypt_fname_encrypted_size() take an fscrypt_policy instead
  of an inode.

Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c           |  11 ++--
 fs/crypto/fscrypt_private.h |   6 +-
 fs/crypto/hooks.c           |  30 ++++------
 fs/crypto/keysetup.c        |  33 +++--------
 fs/crypto/policy.c          | 113 ++++++++++++++++++++++--------------
 fs/ext4/ext4.h              |   6 +-
 fs/ext4/super.c             |  17 +++---
 fs/f2fs/f2fs.h              |   2 +-
 fs/f2fs/super.c             |  16 ++---
 include/linux/fscrypt.h     |  40 +++++--------
 10 files changed, 134 insertions(+), 140 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 47bcfddb278ba..eb13408b50a70 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -242,11 +242,11 @@ static int base64_decode(const char *src, int len, u8 *dst)
 	return cp - dst;
 }
 
-bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
-				  u32 max_len, u32 *encrypted_len_ret)
+bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
+				  u32 orig_len, u32 max_len,
+				  u32 *encrypted_len_ret)
 {
-	const struct fscrypt_info *ci = inode->i_crypt_info;
-	int padding = 4 << (fscrypt_policy_flags(&ci->ci_policy) &
+	int padding = 4 << (fscrypt_policy_flags(policy) &
 			    FSCRYPT_POLICY_FLAGS_PAD_MASK);
 	u32 encrypted_len;
 
@@ -418,7 +418,8 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 		return ret;
 
 	if (fscrypt_has_encryption_key(dir)) {
-		if (!fscrypt_fname_encrypted_size(dir, iname->len,
+		if (!fscrypt_fname_encrypted_size(&dir->i_crypt_info->ci_policy,
+						  iname->len,
 						  dir->i_sb->s_cop->max_namelen,
 						  &fname->crypto_buf.len))
 			return -ENAMETOOLONG;
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index ac3352086ee44..4f5806a3b73d7 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -291,8 +291,9 @@ void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
 /* fname.c */
 int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
 			  u8 *out, unsigned int olen);
-bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
-				  u32 max_len, u32 *encrypted_len_ret);
+bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
+				  u32 orig_len, u32 max_len,
+				  u32 *encrypted_len_ret);
 extern const struct dentry_operations fscrypt_d_ops;
 
 /* hkdf.c */
@@ -592,5 +593,6 @@ bool fscrypt_supported_policy(const union fscrypt_policy *policy_u,
 int fscrypt_policy_from_context(union fscrypt_policy *policy_u,
 				const union fscrypt_context *ctx_u,
 				int ctx_size);
+const union fscrypt_policy *fscrypt_policy_to_inherit(struct inode *dir);
 
 #endif /* _FSCRYPT_PRIVATE_H */
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index a399c54947f28..42f5ee9f592d1 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -193,30 +193,24 @@ int fscrypt_prepare_symlink(struct inode *dir, const char *target,
 			    unsigned int len, unsigned int max_len,
 			    struct fscrypt_str *disk_link)
 {
-	int err;
+	const union fscrypt_policy *policy;
 
-	if (!IS_ENCRYPTED(dir) && !fscrypt_get_dummy_context(dir->i_sb)) {
+	/*
+	 * To calculate the size of the encrypted symlink target we need to know
+	 * the amount of NUL padding, which is determined by the flags set in
+	 * the encryption policy which will be inherited from the directory.
+	 */
+	policy = fscrypt_policy_to_inherit(dir);
+	if (policy == NULL) {
+		/* Not encrypted */
 		disk_link->name = (unsigned char *)target;
 		disk_link->len = len + 1;
 		if (disk_link->len > max_len)
 			return -ENAMETOOLONG;
 		return 0;
 	}
-
-	/*
-	 * To calculate the size of the encrypted symlink target we need to know
-	 * the amount of NUL padding, which is determined by the flags set in
-	 * the encryption policy which will be inherited from the directory.
-	 * The easiest way to get access to this is to just load the directory's
-	 * fscrypt_info, since we'll need it to create the dir_entry anyway.
-	 *
-	 * Note: in test_dummy_encryption mode, @dir may be unencrypted.
-	 */
-	err = fscrypt_get_encryption_info(dir);
-	if (err)
-		return err;
-	if (!fscrypt_has_encryption_key(dir))
-		return -ENOKEY;
+	if (IS_ERR(policy))
+		return PTR_ERR(policy);
 
 	/*
 	 * Calculate the size of the encrypted symlink and verify it won't
@@ -229,7 +223,7 @@ int fscrypt_prepare_symlink(struct inode *dir, const char *target,
 	 * counting it (even though it is meaningless for ciphertext) is simpler
 	 * for now since filesystems will assume it is there and subtract it.
 	 */
-	if (!fscrypt_fname_encrypted_size(dir, len,
+	if (!fscrypt_fname_encrypted_size(policy, len,
 					  max_len - sizeof(struct fscrypt_symlink_data),
 					  &disk_link->len))
 		return -ENAMETOOLONG;
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 47f19061ba10e..d3c3e5d9b41f7 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -547,7 +547,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 
 /**
  * fscrypt_get_encryption_info() - set up an inode's encryption key
- * @inode: the inode to set up the key for
+ * @inode: the inode to set up the key for.  Must be encrypted.
  *
  * Set up ->i_crypt_info, if it hasn't already been done.
  *
@@ -569,18 +569,8 @@ int fscrypt_get_encryption_info(struct inode *inode)
 
 	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
 	if (res < 0) {
-		const union fscrypt_context *dummy_ctx =
-			fscrypt_get_dummy_context(inode->i_sb);
-
-		if (IS_ENCRYPTED(inode) || !dummy_ctx) {
-			fscrypt_warn(inode,
-				     "Error %d getting encryption context",
-				     res);
-			return res;
-		}
-		/* Fake up a context for an unencrypted directory */
-		res = fscrypt_context_size(dummy_ctx);
-		memcpy(&ctx, dummy_ctx, res);
+		fscrypt_warn(inode, "Error %d getting encryption context", res);
+		return res;
 	}
 
 	res = fscrypt_policy_from_context(&policy, &ctx, res);
@@ -627,17 +617,14 @@ EXPORT_SYMBOL(fscrypt_get_encryption_info);
 int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
 			      bool *encrypt_ret)
 {
-	int err;
+	const union fscrypt_policy *policy;
 	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
 
-	if (!IS_ENCRYPTED(dir) && fscrypt_get_dummy_context(dir->i_sb) == NULL)
+	policy = fscrypt_policy_to_inherit(dir);
+	if (policy == NULL)
 		return 0;
-
-	err = fscrypt_get_encryption_info(dir);
-	if (err)
-		return err;
-	if (!fscrypt_has_encryption_key(dir))
-		return -ENOKEY;
+	if (IS_ERR(policy))
+		return PTR_ERR(policy);
 
 	if (WARN_ON_ONCE(inode->i_mode == 0))
 		return -EINVAL;
@@ -654,9 +641,7 @@ int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
 	*encrypt_ret = true;
 
 	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
-	return fscrypt_setup_encryption_info(inode,
-					     &dir->i_crypt_info->ci_policy,
-					     nonce,
+	return fscrypt_setup_encryption_info(inode, policy, nonce,
 					     IS_CASEFOLDED(dir) &&
 					     S_ISDIR(inode->i_mode));
 }
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 4ff893f7b030a..97cf07543651f 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -32,6 +32,14 @@ bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
 	return !memcmp(policy1, policy2, fscrypt_policy_size(policy1));
 }
 
+static const union fscrypt_policy *
+fscrypt_get_dummy_policy(struct super_block *sb)
+{
+	if (!sb->s_cop->get_dummy_policy)
+		return NULL;
+	return sb->s_cop->get_dummy_policy(sb);
+}
+
 static bool fscrypt_valid_enc_modes(u32 contents_mode, u32 filenames_mode)
 {
 	if (contents_mode == FSCRYPT_MODE_AES_256_XTS &&
@@ -628,6 +636,25 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
 }
 EXPORT_SYMBOL(fscrypt_has_permitted_context);
 
+/*
+ * Return the encryption policy that new files in the directory will inherit, or
+ * NULL if none, or an ERR_PTR() on error.  If the directory is encrypted, also
+ * ensure that its key is set up, so that the new filename can be encrypted.
+ */
+const union fscrypt_policy *fscrypt_policy_to_inherit(struct inode *dir)
+{
+	int err;
+
+	if (IS_ENCRYPTED(dir)) {
+		err = fscrypt_require_key(dir);
+		if (err)
+			return ERR_PTR(err);
+		return &dir->i_crypt_info->ci_policy;
+	}
+
+	return fscrypt_get_dummy_policy(dir->i_sb);
+}
+
 /**
  * fscrypt_set_context() - Set the fscrypt context of a new inode
  * @inode: a new inode
@@ -672,31 +699,28 @@ EXPORT_SYMBOL_GPL(fscrypt_set_context);
  * @sb: the filesystem on which test_dummy_encryption is being specified
  * @arg: the argument to the test_dummy_encryption option.
  *	 If no argument was specified, then @arg->from == NULL.
- * @dummy_ctx: the filesystem's current dummy context (input/output, see below)
+ * @dummy_policy: the filesystem's current dummy policy (input/output, see
+ *		  below)
  *
  * Handle the test_dummy_encryption mount option by creating a dummy encryption
- * context, saving it in @dummy_ctx, and adding the corresponding dummy
- * encryption key to the filesystem.  If the @dummy_ctx is already set, then
+ * policy, saving it in @dummy_policy, and adding the corresponding dummy
+ * encryption key to the filesystem.  If the @dummy_policy is already set, then
  * instead validate that it matches @arg.  Don't support changing it via
  * remount, as that is difficult to do safely.
  *
- * The reason we use an fscrypt_context rather than an fscrypt_policy is because
- * we mustn't generate a new nonce each time we access a dummy-encrypted
- * directory, as that would change the way filenames are encrypted.
- *
- * Return: 0 on success (dummy context set, or the same context is already set);
- *         -EEXIST if a different dummy context is already set;
+ * Return: 0 on success (dummy policy set, or the same policy is already set);
+ *         -EEXIST if a different dummy policy is already set;
  *         or another -errno value.
  */
 int fscrypt_set_test_dummy_encryption(struct super_block *sb,
 				      const substring_t *arg,
-				      struct fscrypt_dummy_context *dummy_ctx)
+				      struct fscrypt_dummy_policy *dummy_policy)
 {
 	const char *argstr = "v2";
 	const char *argstr_to_free = NULL;
 	struct fscrypt_key_specifier key_spec = { 0 };
 	int version;
-	union fscrypt_context *ctx = NULL;
+	union fscrypt_policy *policy = NULL;
 	int err;
 
 	if (arg->from) {
@@ -706,12 +730,12 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb,
 	}
 
 	if (!strcmp(argstr, "v1")) {
-		version = FSCRYPT_CONTEXT_V1;
+		version = FSCRYPT_POLICY_V1;
 		key_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
 		memset(key_spec.u.descriptor, 0x42,
 		       FSCRYPT_KEY_DESCRIPTOR_SIZE);
 	} else if (!strcmp(argstr, "v2")) {
-		version = FSCRYPT_CONTEXT_V2;
+		version = FSCRYPT_POLICY_V2;
 		key_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
 		/* key_spec.u.identifier gets filled in when adding the key */
 	} else {
@@ -719,21 +743,8 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb,
 		goto out;
 	}
 
-	if (dummy_ctx->ctx) {
-		/*
-		 * Note: if we ever make test_dummy_encryption support
-		 * specifying other encryption settings, such as the encryption
-		 * modes, we'll need to compare those settings here.
-		 */
-		if (dummy_ctx->ctx->version == version)
-			err = 0;
-		else
-			err = -EEXIST;
-		goto out;
-	}
-
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
-	if (!ctx) {
+	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
+	if (!policy) {
 		err = -ENOMEM;
 		goto out;
 	}
@@ -742,18 +753,18 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb,
 	if (err)
 		goto out;
 
-	ctx->version = version;
-	switch (ctx->version) {
-	case FSCRYPT_CONTEXT_V1:
-		ctx->v1.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
-		ctx->v1.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
-		memcpy(ctx->v1.master_key_descriptor, key_spec.u.descriptor,
+	policy->version = version;
+	switch (policy->version) {
+	case FSCRYPT_POLICY_V1:
+		policy->v1.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
+		policy->v1.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
+		memcpy(policy->v1.master_key_descriptor, key_spec.u.descriptor,
 		       FSCRYPT_KEY_DESCRIPTOR_SIZE);
 		break;
-	case FSCRYPT_CONTEXT_V2:
-		ctx->v2.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
-		ctx->v2.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
-		memcpy(ctx->v2.master_key_identifier, key_spec.u.identifier,
+	case FSCRYPT_POLICY_V2:
+		policy->v2.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
+		policy->v2.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
+		memcpy(policy->v2.master_key_identifier, key_spec.u.identifier,
 		       FSCRYPT_KEY_IDENTIFIER_SIZE);
 		break;
 	default:
@@ -761,11 +772,19 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb,
 		err = -EINVAL;
 		goto out;
 	}
-	dummy_ctx->ctx = ctx;
-	ctx = NULL;
+
+	if (dummy_policy->policy) {
+		if (fscrypt_policies_equal(policy, dummy_policy->policy))
+			err = 0;
+		else
+			err = -EEXIST;
+		goto out;
+	}
+	dummy_policy->policy = policy;
+	policy = NULL;
 	err = 0;
 out:
-	kfree(ctx);
+	kfree(policy);
 	kfree(argstr_to_free);
 	return err;
 }
@@ -783,10 +802,16 @@ EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
 void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
 					struct super_block *sb)
 {
-	const union fscrypt_context *ctx = fscrypt_get_dummy_context(sb);
+	const union fscrypt_policy *policy = fscrypt_get_dummy_policy(sb);
+	int vers;
 
-	if (!ctx)
+	if (!policy)
 		return;
-	seq_printf(seq, "%ctest_dummy_encryption=v%d", sep, ctx->version);
+
+	vers = policy->version;
+	if (vers == FSCRYPT_POLICY_V1) /* Handle numbering quirk */
+		vers = 1;
+
+	seq_printf(seq, "%ctest_dummy_encryption=v%d", sep, vers);
 }
 EXPORT_SYMBOL_GPL(fscrypt_show_test_dummy_encryption);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 523e00d7b3924..f9a692c0a66c3 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1401,7 +1401,7 @@ struct ext4_super_block {
 #define EXT4_MF_FS_ABORTED		0x0002	/* Fatal error detected */
 
 #ifdef CONFIG_FS_ENCRYPTION
-#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_ctx.ctx != NULL)
+#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
 #else
 #define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
 #endif
@@ -1596,8 +1596,8 @@ struct ext4_sb_info {
 	atomic_t s_warning_count;
 	atomic_t s_msg_count;
 
-	/* Encryption context for '-o test_dummy_encryption' */
-	struct fscrypt_dummy_context s_dummy_enc_ctx;
+	/* Encryption policy for '-o test_dummy_encryption' */
+	struct fscrypt_dummy_policy s_dummy_enc_policy;
 
 	/*
 	 * Barrier between writepages ops and changing any inode's JOURNAL_DATA
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ea425b49b3456..7e77722406e2f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1104,7 +1104,7 @@ static void ext4_put_super(struct super_block *sb)
 		crypto_free_shash(sbi->s_chksum_driver);
 	kfree(sbi->s_blockgroup_lock);
 	fs_put_dax(sbi->s_daxdev);
-	fscrypt_free_dummy_context(&sbi->s_dummy_enc_ctx);
+	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
 #ifdef CONFIG_UNICODE
 	utf8_unload(sbi->s_encoding);
 #endif
@@ -1392,10 +1392,10 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	return res;
 }
 
-static const union fscrypt_context *
-ext4_get_dummy_context(struct super_block *sb)
+static const union fscrypt_policy *
+ext4_get_dummy_policy(struct super_block *sb)
 {
-	return EXT4_SB(sb)->s_dummy_enc_ctx.ctx;
+	return EXT4_SB(sb)->s_dummy_enc_policy.policy;
 }
 
 static bool ext4_has_stable_inodes(struct super_block *sb)
@@ -1414,7 +1414,7 @@ static const struct fscrypt_operations ext4_cryptops = {
 	.key_prefix		= "ext4:",
 	.get_context		= ext4_get_context,
 	.set_context		= ext4_set_context,
-	.get_dummy_context	= ext4_get_dummy_context,
+	.get_dummy_policy	= ext4_get_dummy_policy,
 	.empty_dir		= ext4_empty_dir,
 	.max_namelen		= EXT4_NAME_LEN,
 	.has_stable_inodes	= ext4_has_stable_inodes,
@@ -1888,12 +1888,13 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb,
 	 * needed to allow it to be set or changed during remount.  We do allow
 	 * it to be specified during remount, but only if there is no change.
 	 */
-	if (is_remount && !sbi->s_dummy_enc_ctx.ctx) {
+	if (is_remount && !sbi->s_dummy_enc_policy.policy) {
 		ext4_msg(sb, KERN_WARNING,
 			 "Can't set test_dummy_encryption on remount");
 		return -1;
 	}
-	err = fscrypt_set_test_dummy_encryption(sb, arg, &sbi->s_dummy_enc_ctx);
+	err = fscrypt_set_test_dummy_encryption(sb, arg,
+						&sbi->s_dummy_enc_policy);
 	if (err) {
 		if (err == -EEXIST)
 			ext4_msg(sb, KERN_WARNING,
@@ -4935,7 +4936,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
 		kfree(get_qf_name(sb, sbi, i));
 #endif
-	fscrypt_free_dummy_context(&sbi->s_dummy_enc_ctx);
+	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
 	ext4_blkdev_remove(sbi);
 	brelse(bh);
 out_fail:
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 0503371f88ed4..7c089ff7ff943 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -138,7 +138,7 @@ struct f2fs_mount_info {
 	int fsync_mode;			/* fsync policy */
 	int fs_mode;			/* fs mode: LFS or ADAPTIVE */
 	int bggc_mode;			/* bggc mode: off, on or sync */
-	struct fscrypt_dummy_context dummy_enc_ctx; /* test dummy encryption */
+	struct fscrypt_dummy_policy dummy_enc_policy; /* test dummy encryption */
 	block_t unusable_cap_perc;	/* percentage for cap */
 	block_t unusable_cap;		/* Amount of space allowed to be
 					 * unusable when disabling checkpoint
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index dfa072fa80815..f2b3d1a279fb7 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -433,12 +433,12 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
 	 * needed to allow it to be set or changed during remount.  We do allow
 	 * it to be specified during remount, but only if there is no change.
 	 */
-	if (is_remount && !F2FS_OPTION(sbi).dummy_enc_ctx.ctx) {
+	if (is_remount && !F2FS_OPTION(sbi).dummy_enc_policy.policy) {
 		f2fs_warn(sbi, "Can't set test_dummy_encryption on remount");
 		return -EINVAL;
 	}
 	err = fscrypt_set_test_dummy_encryption(
-		sb, arg, &F2FS_OPTION(sbi).dummy_enc_ctx);
+		sb, arg, &F2FS_OPTION(sbi).dummy_enc_policy);
 	if (err) {
 		if (err == -EEXIST)
 			f2fs_warn(sbi,
@@ -1275,7 +1275,7 @@ static void f2fs_put_super(struct super_block *sb)
 	for (i = 0; i < MAXQUOTAS; i++)
 		kfree(F2FS_OPTION(sbi).s_qf_names[i]);
 #endif
-	fscrypt_free_dummy_context(&F2FS_OPTION(sbi).dummy_enc_ctx);
+	fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy);
 	destroy_percpu_info(sbi);
 	for (i = 0; i < NR_PAGE_TYPE; i++)
 		kvfree(sbi->write_io[i]);
@@ -2482,10 +2482,10 @@ static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len,
 				ctx, len, fs_data, XATTR_CREATE);
 }
 
-static const union fscrypt_context *
-f2fs_get_dummy_context(struct super_block *sb)
+static const union fscrypt_policy *
+f2fs_get_dummy_policy(struct super_block *sb)
 {
-	return F2FS_OPTION(F2FS_SB(sb)).dummy_enc_ctx.ctx;
+	return F2FS_OPTION(F2FS_SB(sb)).dummy_enc_policy.policy;
 }
 
 static bool f2fs_has_stable_inodes(struct super_block *sb)
@@ -2523,7 +2523,7 @@ static const struct fscrypt_operations f2fs_cryptops = {
 	.key_prefix		= "f2fs:",
 	.get_context		= f2fs_get_context,
 	.set_context		= f2fs_set_context,
-	.get_dummy_context	= f2fs_get_dummy_context,
+	.get_dummy_policy	= f2fs_get_dummy_policy,
 	.empty_dir		= f2fs_empty_dir,
 	.max_namelen		= F2FS_NAME_LEN,
 	.has_stable_inodes	= f2fs_has_stable_inodes,
@@ -3864,7 +3864,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	for (i = 0; i < MAXQUOTAS; i++)
 		kfree(F2FS_OPTION(sbi).s_qf_names[i]);
 #endif
-	fscrypt_free_dummy_context(&F2FS_OPTION(sbi).dummy_enc_ctx);
+	fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy);
 	kvfree(options);
 free_sb_buf:
 	kfree(raw_super);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 39e7397a3f103..b3b0c5675c6b1 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -21,7 +21,7 @@
 
 #define FS_CRYPTO_BLOCK_SIZE		16
 
-union fscrypt_context;
+union fscrypt_policy;
 struct fscrypt_info;
 struct seq_file;
 
@@ -62,8 +62,7 @@ struct fscrypt_operations {
 	int (*get_context)(struct inode *inode, void *ctx, size_t len);
 	int (*set_context)(struct inode *inode, const void *ctx, size_t len,
 			   void *fs_data);
-	const union fscrypt_context *(*get_dummy_context)(
-		struct super_block *sb);
+	const union fscrypt_policy *(*get_dummy_policy)(struct super_block *sb);
 	bool (*empty_dir)(struct inode *inode);
 	unsigned int max_namelen;
 	bool (*has_stable_inodes)(struct super_block *sb);
@@ -101,14 +100,6 @@ static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
 	return IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode);
 }
 
-static inline const union fscrypt_context *
-fscrypt_get_dummy_context(struct super_block *sb)
-{
-	if (!sb->s_cop->get_dummy_context)
-		return NULL;
-	return sb->s_cop->get_dummy_context(sb);
-}
-
 /*
  * When d_splice_alias() moves a directory's encrypted alias to its decrypted
  * alias as a result of the encryption key being added, DCACHE_ENCRYPTED_NAME
@@ -158,20 +149,21 @@ 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);
 
-struct fscrypt_dummy_context {
-	const union fscrypt_context *ctx;
+struct fscrypt_dummy_policy {
+	const union fscrypt_policy *policy;
 };
 
-int fscrypt_set_test_dummy_encryption(struct super_block *sb,
-				      const substring_t *arg,
-				      struct fscrypt_dummy_context *dummy_ctx);
+int fscrypt_set_test_dummy_encryption(
+				struct super_block *sb,
+				const substring_t *arg,
+				struct fscrypt_dummy_policy *dummy_policy);
 void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
 					struct super_block *sb);
 static inline void
-fscrypt_free_dummy_context(struct fscrypt_dummy_context *dummy_ctx)
+fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
 {
-	kfree(dummy_ctx->ctx);
-	dummy_ctx->ctx = NULL;
+	kfree(dummy_policy->policy);
+	dummy_policy->policy = NULL;
 }
 
 /* keyring.c */
@@ -250,12 +242,6 @@ static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
 	return false;
 }
 
-static inline const union fscrypt_context *
-fscrypt_get_dummy_context(struct super_block *sb)
-{
-	return NULL;
-}
-
 static inline void fscrypt_handle_d_move(struct dentry *dentry)
 {
 }
@@ -346,7 +332,7 @@ static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
 	return -EOPNOTSUPP;
 }
 
-struct fscrypt_dummy_context {
+struct fscrypt_dummy_policy {
 };
 
 static inline void fscrypt_show_test_dummy_encryption(struct seq_file *seq,
@@ -356,7 +342,7 @@ static inline void fscrypt_show_test_dummy_encryption(struct seq_file *seq,
 }
 
 static inline void
-fscrypt_free_dummy_context(struct fscrypt_dummy_context *dummy_ctx)
+fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
 {
 }
 
-- 
2.28.0


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

* [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *'
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (11 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 12/13] fscrypt: handle test_dummy_encryption in more logical way Eric Biggers
@ 2020-09-17  4:11 ` Eric Biggers
  2020-09-17 12:32   ` Jeff Layton
  2020-09-21 22:35 ` [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
  13 siblings, 1 reply; 20+ messages in thread
From: Eric Biggers @ 2020-09-17  4:11 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Jeff Layton,
	Daniel Rosenberg

From: Eric Biggers <ebiggers@google.com>

fscrypt_set_test_dummy_encryption() requires that the optional argument
to the test_dummy_encryption mount option be specified as a substring_t.
That doesn't work well with filesystems that use the new mount API,
since the new way of parsing mount options doesn't use substring_t.

Make it take the argument as a 'const char *' instead.

Instead of moving the match_strdup() into the callers in ext4 and f2fs,
make them just use arg->from directly.  Since the pattern is
"test_dummy_encryption=%s", the argument will be null-terminated.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/policy.c      | 20 ++++++--------------
 fs/ext4/super.c         |  2 +-
 fs/f2fs/super.c         |  2 +-
 include/linux/fscrypt.h |  5 +----
 4 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 97cf07543651f..4441d9944b9ef 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -697,8 +697,7 @@ EXPORT_SYMBOL_GPL(fscrypt_set_context);
 /**
  * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption'
  * @sb: the filesystem on which test_dummy_encryption is being specified
- * @arg: the argument to the test_dummy_encryption option.
- *	 If no argument was specified, then @arg->from == NULL.
+ * @arg: the argument to the test_dummy_encryption option.  May be NULL.
  * @dummy_policy: the filesystem's current dummy policy (input/output, see
  *		  below)
  *
@@ -712,29 +711,23 @@ EXPORT_SYMBOL_GPL(fscrypt_set_context);
  *         -EEXIST if a different dummy policy is already set;
  *         or another -errno value.
  */
-int fscrypt_set_test_dummy_encryption(struct super_block *sb,
-				      const substring_t *arg,
+int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
 				      struct fscrypt_dummy_policy *dummy_policy)
 {
-	const char *argstr = "v2";
-	const char *argstr_to_free = NULL;
 	struct fscrypt_key_specifier key_spec = { 0 };
 	int version;
 	union fscrypt_policy *policy = NULL;
 	int err;
 
-	if (arg->from) {
-		argstr = argstr_to_free = match_strdup(arg);
-		if (!argstr)
-			return -ENOMEM;
-	}
+	if (!arg)
+		arg = "v2";
 
-	if (!strcmp(argstr, "v1")) {
+	if (!strcmp(arg, "v1")) {
 		version = FSCRYPT_POLICY_V1;
 		key_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
 		memset(key_spec.u.descriptor, 0x42,
 		       FSCRYPT_KEY_DESCRIPTOR_SIZE);
-	} else if (!strcmp(argstr, "v2")) {
+	} else if (!strcmp(arg, "v2")) {
 		version = FSCRYPT_POLICY_V2;
 		key_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
 		/* key_spec.u.identifier gets filled in when adding the key */
@@ -785,7 +778,6 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb,
 	err = 0;
 out:
 	kfree(policy);
-	kfree(argstr_to_free);
 	return err;
 }
 EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7e77722406e2f..ed5624285a475 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1893,7 +1893,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb,
 			 "Can't set test_dummy_encryption on remount");
 		return -1;
 	}
-	err = fscrypt_set_test_dummy_encryption(sb, arg,
+	err = fscrypt_set_test_dummy_encryption(sb, arg->from,
 						&sbi->s_dummy_enc_policy);
 	if (err) {
 		if (err == -EEXIST)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index f2b3d1a279fb7..c72d22c0c52e7 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -438,7 +438,7 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
 		return -EINVAL;
 	}
 	err = fscrypt_set_test_dummy_encryption(
-		sb, arg, &F2FS_OPTION(sbi).dummy_enc_policy);
+		sb, arg->from, &F2FS_OPTION(sbi).dummy_enc_policy);
 	if (err) {
 		if (err == -EEXIST)
 			f2fs_warn(sbi,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index b3b0c5675c6b1..fc67c4cbaa968 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -15,7 +15,6 @@
 
 #include <linux/fs.h>
 #include <linux/mm.h>
-#include <linux/parser.h>
 #include <linux/slab.h>
 #include <uapi/linux/fscrypt.h>
 
@@ -153,9 +152,7 @@ struct fscrypt_dummy_policy {
 	const union fscrypt_policy *policy;
 };
 
-int fscrypt_set_test_dummy_encryption(
-				struct super_block *sb,
-				const substring_t *arg,
+int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
 				struct fscrypt_dummy_policy *dummy_policy);
 void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
 					struct super_block *sb);
-- 
2.28.0


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

* Re: [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *'
  2020-09-17  4:11 ` [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *' Eric Biggers
@ 2020-09-17 12:32   ` Jeff Layton
  2020-09-17 15:29     ` Eric Biggers
  0 siblings, 1 reply; 20+ messages in thread
From: Jeff Layton @ 2020-09-17 12:32 UTC (permalink / raw)
  To: Eric Biggers, linux-fscrypt
  Cc: linux-ext4, linux-f2fs-devel, linux-mtd, ceph-devel, Daniel Rosenberg

On Wed, 2020-09-16 at 21:11 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> fscrypt_set_test_dummy_encryption() requires that the optional argument
> to the test_dummy_encryption mount option be specified as a substring_t.
> That doesn't work well with filesystems that use the new mount API,
> since the new way of parsing mount options doesn't use substring_t.
> 
> Make it take the argument as a 'const char *' instead.
> 
> Instead of moving the match_strdup() into the callers in ext4 and f2fs,
> make them just use arg->from directly.  Since the pattern is
> "test_dummy_encryption=%s", the argument will be null-terminated.
> 

Are you sure about that? I thought the point of substring_t was to give
you a token from the string without null terminating it.

ISTM that when you just pass in ->from, you might end up with trailing
arguments in your string like this. e.g.:

    "v2,foo,bar,baz"

...and then that might fail to match properly
in fscrypt_set_test_dummy_encryption.

> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/crypto/policy.c      | 20 ++++++--------------
>  fs/ext4/super.c         |  2 +-
>  fs/f2fs/super.c         |  2 +-
>  include/linux/fscrypt.h |  5 +----
>  4 files changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> index 97cf07543651f..4441d9944b9ef 100644
> --- a/fs/crypto/policy.c
> +++ b/fs/crypto/policy.c
> @@ -697,8 +697,7 @@ EXPORT_SYMBOL_GPL(fscrypt_set_context);
>  /**
>   * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption'
>   * @sb: the filesystem on which test_dummy_encryption is being specified
> - * @arg: the argument to the test_dummy_encryption option.
> - *	 If no argument was specified, then @arg->from == NULL.
> + * @arg: the argument to the test_dummy_encryption option.  May be NULL.
>   * @dummy_policy: the filesystem's current dummy policy (input/output, see
>   *		  below)
>   *
> @@ -712,29 +711,23 @@ EXPORT_SYMBOL_GPL(fscrypt_set_context);
>   *         -EEXIST if a different dummy policy is already set;
>   *         or another -errno value.
>   */
> -int fscrypt_set_test_dummy_encryption(struct super_block *sb,
> -				      const substring_t *arg,
> +int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
>  				      struct fscrypt_dummy_policy *dummy_policy)
>  {
> -	const char *argstr = "v2";
> -	const char *argstr_to_free = NULL;
>  	struct fscrypt_key_specifier key_spec = { 0 };
>  	int version;
>  	union fscrypt_policy *policy = NULL;
>  	int err;
>  
> -	if (arg->from) {
> -		argstr = argstr_to_free = match_strdup(arg);
> -		if (!argstr)
> -			return -ENOMEM;
> -	}
> +	if (!arg)
> +		arg = "v2";
>  
> -	if (!strcmp(argstr, "v1")) {
> +	if (!strcmp(arg, "v1")) {
>  		version = FSCRYPT_POLICY_V1;
>  		key_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
>  		memset(key_spec.u.descriptor, 0x42,
>  		       FSCRYPT_KEY_DESCRIPTOR_SIZE);
> -	} else if (!strcmp(argstr, "v2")) {
> +	} else if (!strcmp(arg, "v2")) {
>  		version = FSCRYPT_POLICY_V2;
>  		key_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
>  		/* key_spec.u.identifier gets filled in when adding the key */
> @@ -785,7 +778,6 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb,
>  	err = 0;
>  out:
>  	kfree(policy);
> -	kfree(argstr_to_free);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7e77722406e2f..ed5624285a475 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1893,7 +1893,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb,
>  			 "Can't set test_dummy_encryption on remount");
>  		return -1;
>  	}
> -	err = fscrypt_set_test_dummy_encryption(sb, arg,
> +	err = fscrypt_set_test_dummy_encryption(sb, arg->from,
>  						&sbi->s_dummy_enc_policy);
>  	if (err) {
>  		if (err == -EEXIST)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index f2b3d1a279fb7..c72d22c0c52e7 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -438,7 +438,7 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
>  		return -EINVAL;
>  	}
>  	err = fscrypt_set_test_dummy_encryption(
> -		sb, arg, &F2FS_OPTION(sbi).dummy_enc_policy);
> +		sb, arg->from, &F2FS_OPTION(sbi).dummy_enc_policy);
>  	if (err) {
>  		if (err == -EEXIST)
>  			f2fs_warn(sbi,
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index b3b0c5675c6b1..fc67c4cbaa968 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -15,7 +15,6 @@
>  
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> -#include <linux/parser.h>
>  #include <linux/slab.h>
>  #include <uapi/linux/fscrypt.h>
>  
> @@ -153,9 +152,7 @@ struct fscrypt_dummy_policy {
>  	const union fscrypt_policy *policy;
>  };
>  
> -int fscrypt_set_test_dummy_encryption(
> -				struct super_block *sb,
> -				const substring_t *arg,
> +int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
>  				struct fscrypt_dummy_policy *dummy_policy);
>  void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
>  					struct super_block *sb);

-- 
Jeff Layton <jlayton@kernel.org>


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

* Re: [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *'
  2020-09-17 12:32   ` Jeff Layton
@ 2020-09-17 15:29     ` Eric Biggers
  2020-09-17 16:33       ` Jeff Layton
  0 siblings, 1 reply; 20+ messages in thread
From: Eric Biggers @ 2020-09-17 15:29 UTC (permalink / raw)
  To: Jeff Layton
  Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, linux-mtd,
	ceph-devel, Daniel Rosenberg

On Thu, Sep 17, 2020 at 08:32:39AM -0400, Jeff Layton wrote:
> On Wed, 2020-09-16 at 21:11 -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > fscrypt_set_test_dummy_encryption() requires that the optional argument
> > to the test_dummy_encryption mount option be specified as a substring_t.
> > That doesn't work well with filesystems that use the new mount API,
> > since the new way of parsing mount options doesn't use substring_t.
> > 
> > Make it take the argument as a 'const char *' instead.
> > 
> > Instead of moving the match_strdup() into the callers in ext4 and f2fs,
> > make them just use arg->from directly.  Since the pattern is
> > "test_dummy_encryption=%s", the argument will be null-terminated.
> > 
> 
> Are you sure about that? I thought the point of substring_t was to give
> you a token from the string without null terminating it.
> 
> ISTM that when you just pass in ->from, you might end up with trailing
> arguments in your string like this. e.g.:
> 
>     "v2,foo,bar,baz"
> 
> ...and then that might fail to match properly
> in fscrypt_set_test_dummy_encryption.
> 

Yes I'm sure, and I had also tested it.  The use of match_token() here is to
parse one null-terminated mount option at a time.

The reason that match_token() can return multiple substrings is that the pattern
might be something like "foo=%d:%d".

But here it's just "test_dummy_encryption=%s". "%s" matches until end-of-string.

- Eric

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

* Re: [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *'
  2020-09-17 15:29     ` Eric Biggers
@ 2020-09-17 16:33       ` Jeff Layton
  0 siblings, 0 replies; 20+ messages in thread
From: Jeff Layton @ 2020-09-17 16:33 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, linux-mtd,
	ceph-devel, Daniel Rosenberg

On Thu, 2020-09-17 at 08:29 -0700, Eric Biggers wrote:
> On Thu, Sep 17, 2020 at 08:32:39AM -0400, Jeff Layton wrote:
> > On Wed, 2020-09-16 at 21:11 -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > fscrypt_set_test_dummy_encryption() requires that the optional argument
> > > to the test_dummy_encryption mount option be specified as a substring_t.
> > > That doesn't work well with filesystems that use the new mount API,
> > > since the new way of parsing mount options doesn't use substring_t.
> > > 
> > > Make it take the argument as a 'const char *' instead.
> > > 
> > > Instead of moving the match_strdup() into the callers in ext4 and f2fs,
> > > make them just use arg->from directly.  Since the pattern is
> > > "test_dummy_encryption=%s", the argument will be null-terminated.
> > > 
> > 
> > Are you sure about that? I thought the point of substring_t was to give
> > you a token from the string without null terminating it.
> > 
> > ISTM that when you just pass in ->from, you might end up with trailing
> > arguments in your string like this. e.g.:
> > 
> >     "v2,foo,bar,baz"
> > 
> > ...and then that might fail to match properly
> > in fscrypt_set_test_dummy_encryption.
> > 
> 
> Yes I'm sure, and I had also tested it.  The use of match_token() here is to
> parse one null-terminated mount option at a time.
> 
> The reason that match_token() can return multiple substrings is that the pattern
> might be something like "foo=%d:%d".
> 
> But here it's just "test_dummy_encryption=%s". "%s" matches until end-of-string.

Got it. Thanks for explaining!
-- 
Jeff Layton <jlayton@kernel.org>


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

* Re: [PATCH v3 00/13] fscrypt: improve file creation flow
  2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
                   ` (12 preceding siblings ...)
  2020-09-17  4:11 ` [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *' Eric Biggers
@ 2020-09-21 22:35 ` Eric Biggers
  2020-09-22 11:29   ` Jeff Layton
  13 siblings, 1 reply; 20+ messages in thread
From: Eric Biggers @ 2020-09-21 22:35 UTC (permalink / raw)
  To: linux-fscrypt
  Cc: Daniel Rosenberg, Jeff Layton, linux-f2fs-devel, linux-mtd,
	ceph-devel, linux-ext4

On Wed, Sep 16, 2020 at 09:11:23PM -0700, Eric Biggers wrote:
> Hello,
> 
> This series reworks the implementation of creating new encrypted files
> by introducing new helper functions that allow filesystems to set up the
> inodes' keys earlier, prior to taking too many filesystem locks.
> 
> This fixes deadlocks that are possible during memory reclaim because
> fscrypt_get_encryption_info() isn't GFP_NOFS-safe, yet it's called
> during an ext4 transaction or under f2fs_lock_op().  It also fixes a
> similar deadlock where f2fs can try to recursively lock a page when the
> test_dummy_encryption mount option is in use.
> 
> It also solves an ordering problem that the ceph support for fscrypt
> will have.  For more details about this ordering problem, see the
> discussion on Jeff Layton's RFC patchsets for ceph fscrypt support
> (v1: https://lkml.kernel.org/linux-fscrypt/20200821182813.52570-1-jlayton@kernel.org/T/#u
>  v2: https://lkml.kernel.org/linux-fscrypt/20200904160537.76663-1-jlayton@kernel.org/T/#u
>  v3: https://lkml.kernel.org/linux-fscrypt/20200914191707.380444-1-jlayton@kernel.org/T/#u)
> Note that v3 of the ceph patchset is based on v2 of this patchset.
> 
> Patch 1 adds the above-mentioned new helper functions.  Patches 2-5
> convert ext4, f2fs, and ubifs to use them, and patches 6-9 clean up a
> few things afterwards.
> 
> Finally, patches 10-13 change the implementation of
> test_dummy_encryption to no longer set up an encryption key for
> unencrypted directories, which was confusing and was causing problems.
> 
> This patchset applies to the master branch of
> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git.
> It can also be retrieved from tag "fscrypt-file-creation-v3" of
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git.
> 
> I'm looking to apply this for 5.10; reviews are greatly appreciated!
> 
> Changed v2 => v3:
>   - Added patch that changes fscrypt_set_test_dummy_encryption() to take
>     a 'const char *'.  (Needed by ceph.)
>   - Fixed bug where fscrypt_prepare_new_inode() succeeded even if the
>     new inode's key couldn't be set up.
>   - Fixed bug where fscrypt_prepare_new_inode() wouldn't derive the
>     dirhash key for new casefolded directories.
>   - Made warning messages account for i_ino possibly being 0 now.
> 
> Changed v1 => v2:
>   - Added mention of another deadlock this fixes.
>   - Added patches to improve the test_dummy_encryption implementation.
>   - Dropped an ext4 cleanup patch that can be done separately later.
>   - Lots of small cleanups, and a couple small fixes.
> 
> Eric Biggers (13):
>   fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()
>   ext4: factor out ext4_xattr_credits_for_new_inode()
>   ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context()
>   f2fs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
>   ubifs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
>   fscrypt: adjust logging for in-creation inodes
>   fscrypt: remove fscrypt_inherit_context()
>   fscrypt: require that fscrypt_encrypt_symlink() already has key
>   fscrypt: stop pretending that key setup is nofs-safe
>   fscrypt: make "#define fscrypt_policy" user-only
>   fscrypt: move fscrypt_prepare_symlink() out-of-line
>   fscrypt: handle test_dummy_encryption in more logical way
>   fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char
>     *'

All applied to fscrypt.git#master for 5.10.

I'd still really appreciate more reviews and acks, though.

- Eric

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

* Re: [PATCH v3 00/13] fscrypt: improve file creation flow
  2020-09-21 22:35 ` [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
@ 2020-09-22 11:29   ` Jeff Layton
  2020-09-22 13:50     ` Eric Biggers
  0 siblings, 1 reply; 20+ messages in thread
From: Jeff Layton @ 2020-09-22 11:29 UTC (permalink / raw)
  To: Eric Biggers, linux-fscrypt
  Cc: Daniel Rosenberg, linux-f2fs-devel, linux-mtd, ceph-devel, linux-ext4

On Mon, 2020-09-21 at 15:35 -0700, Eric Biggers wrote:
> On Wed, Sep 16, 2020 at 09:11:23PM -0700, Eric Biggers wrote:
> > Hello,
> > 
> > This series reworks the implementation of creating new encrypted files
> > by introducing new helper functions that allow filesystems to set up the
> > inodes' keys earlier, prior to taking too many filesystem locks.
> > 
> > This fixes deadlocks that are possible during memory reclaim because
> > fscrypt_get_encryption_info() isn't GFP_NOFS-safe, yet it's called
> > during an ext4 transaction or under f2fs_lock_op().  It also fixes a
> > similar deadlock where f2fs can try to recursively lock a page when the
> > test_dummy_encryption mount option is in use.
> > 
> > It also solves an ordering problem that the ceph support for fscrypt
> > will have.  For more details about this ordering problem, see the
> > discussion on Jeff Layton's RFC patchsets for ceph fscrypt support
> > (v1: https://lkml.kernel.org/linux-fscrypt/20200821182813.52570-1-jlayton@kernel.org/T/#u
> >  v2: https://lkml.kernel.org/linux-fscrypt/20200904160537.76663-1-jlayton@kernel.org/T/#u
> >  v3: https://lkml.kernel.org/linux-fscrypt/20200914191707.380444-1-jlayton@kernel.org/T/#u)
> > Note that v3 of the ceph patchset is based on v2 of this patchset.
> > 
> > Patch 1 adds the above-mentioned new helper functions.  Patches 2-5
> > convert ext4, f2fs, and ubifs to use them, and patches 6-9 clean up a
> > few things afterwards.
> > 
> > Finally, patches 10-13 change the implementation of
> > test_dummy_encryption to no longer set up an encryption key for
> > unencrypted directories, which was confusing and was causing problems.
> > 
> > This patchset applies to the master branch of
> > https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git.
> > It can also be retrieved from tag "fscrypt-file-creation-v3" of
> > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git.
> > 
> > I'm looking to apply this for 5.10; reviews are greatly appreciated!
> > 
> > Changed v2 => v3:
> >   - Added patch that changes fscrypt_set_test_dummy_encryption() to take
> >     a 'const char *'.  (Needed by ceph.)
> >   - Fixed bug where fscrypt_prepare_new_inode() succeeded even if the
> >     new inode's key couldn't be set up.
> >   - Fixed bug where fscrypt_prepare_new_inode() wouldn't derive the
> >     dirhash key for new casefolded directories.
> >   - Made warning messages account for i_ino possibly being 0 now.
> > 
> > Changed v1 => v2:
> >   - Added mention of another deadlock this fixes.
> >   - Added patches to improve the test_dummy_encryption implementation.
> >   - Dropped an ext4 cleanup patch that can be done separately later.
> >   - Lots of small cleanups, and a couple small fixes.
> > 
> > Eric Biggers (13):
> >   fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context()
> >   ext4: factor out ext4_xattr_credits_for_new_inode()
> >   ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context()
> >   f2fs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
> >   ubifs: use fscrypt_prepare_new_inode() and fscrypt_set_context()
> >   fscrypt: adjust logging for in-creation inodes
> >   fscrypt: remove fscrypt_inherit_context()
> >   fscrypt: require that fscrypt_encrypt_symlink() already has key
> >   fscrypt: stop pretending that key setup is nofs-safe
> >   fscrypt: make "#define fscrypt_policy" user-only
> >   fscrypt: move fscrypt_prepare_symlink() out-of-line
> >   fscrypt: handle test_dummy_encryption in more logical way
> >   fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char
> >     *'
> 
> All applied to fscrypt.git#master for 5.10.
> 
> I'd still really appreciate more reviews and acks, though.
> 

You can add this to all of the fscrypt: patches. I've tested this under
the ceph patchset and it seems to do the right thing:

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


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

* Re: [PATCH v3 00/13] fscrypt: improve file creation flow
  2020-09-22 11:29   ` Jeff Layton
@ 2020-09-22 13:50     ` Eric Biggers
  0 siblings, 0 replies; 20+ messages in thread
From: Eric Biggers @ 2020-09-22 13:50 UTC (permalink / raw)
  To: Jeff Layton
  Cc: linux-fscrypt, Daniel Rosenberg, linux-f2fs-devel, linux-mtd,
	ceph-devel, linux-ext4

On Tue, Sep 22, 2020 at 07:29:45AM -0400, Jeff Layton wrote:
> > 
> > All applied to fscrypt.git#master for 5.10.
> > 
> > I'd still really appreciate more reviews and acks, though.
> > 
> 
> You can add this to all of the fscrypt: patches. I've tested this under
> the ceph patchset and it seems to do the right thing:
> 
> Acked-by: Jeff Layton <jlayton@kernel.org>
> 

Thanks, added.

- Eric

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

end of thread, other threads:[~2020-09-22 13:50 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17  4:11 [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
2020-09-17  4:11 ` [PATCH v3 01/13] fscrypt: add fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
2020-09-17  4:11 ` [PATCH v3 02/13] ext4: factor out ext4_xattr_credits_for_new_inode() Eric Biggers
2020-09-17  4:11 ` [PATCH v3 03/13] ext4: use fscrypt_prepare_new_inode() and fscrypt_set_context() Eric Biggers
2020-09-17  4:11 ` [PATCH v3 04/13] f2fs: " Eric Biggers
2020-09-17  4:11 ` [PATCH v3 05/13] ubifs: " Eric Biggers
2020-09-17  4:11 ` [PATCH v3 06/13] fscrypt: adjust logging for in-creation inodes Eric Biggers
2020-09-17  4:11 ` [PATCH v3 07/13] fscrypt: remove fscrypt_inherit_context() Eric Biggers
2020-09-17  4:11 ` [PATCH v3 08/13] fscrypt: require that fscrypt_encrypt_symlink() already has key Eric Biggers
2020-09-17  4:11 ` [PATCH v3 09/13] fscrypt: stop pretending that key setup is nofs-safe Eric Biggers
2020-09-17  4:11 ` [PATCH v3 10/13] fscrypt: make "#define fscrypt_policy" user-only Eric Biggers
2020-09-17  4:11 ` [PATCH v3 11/13] fscrypt: move fscrypt_prepare_symlink() out-of-line Eric Biggers
2020-09-17  4:11 ` [PATCH v3 12/13] fscrypt: handle test_dummy_encryption in more logical way Eric Biggers
2020-09-17  4:11 ` [PATCH v3 13/13] fscrypt: make fscrypt_set_test_dummy_encryption() take a 'const char *' Eric Biggers
2020-09-17 12:32   ` Jeff Layton
2020-09-17 15:29     ` Eric Biggers
2020-09-17 16:33       ` Jeff Layton
2020-09-21 22:35 ` [PATCH v3 00/13] fscrypt: improve file creation flow Eric Biggers
2020-09-22 11:29   ` Jeff Layton
2020-09-22 13:50     ` Eric Biggers

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