ext4: Move out crypto ops to ext4_crypto.c

RFC Superseded Ritesh Harjani 2 participants 11 messages Apr 21, 2022 – May 2, 2022

Subsystems (2)

ext4 file system fscrypt: file system level encryption support

lore

Share & tools

lore

Ritesh Harjani

author reviewer

2022-04-21 05:23 UTC #0 lore raw

cover ext4: Move out crypto ops to ext4_crypto.c

Hello,

This is 1st in the series to cleanup ext4/super.c, since it has grown quite large. This moves out crypto related ops and few definitions to fs/ext4/ext4_crypto.c

Testing
=========
1. Tested "-g encrypt" with default configs.
2. Compiled tested on x86 & Power.

Ritesh Harjani (6):
fscrypt: Provide definition of fscrypt_set_test_dummy_encryption
ext4: Move ext4 crypto code to its own file ext4_crypto.c
ext4: Directly opencode ext4_set_test_dummy_encryption
ext4: Cleanup function defs from ext4.h into ext4_crypto.c
ext4: Move all encryption related into a common #ifdef
ext4: Use provided macro for checking dummy_enc_policy

 fs/ext4/Makefile        |   1 +
 fs/ext4/ext4.h          |  81 +++--------------
 fs/ext4/ext4_crypto.c   | 192 ++++++++++++++++++++++++++++++++++++++++
 fs/ext4/super.c         | 158 ++++-----------------------------
 include/linux/fscrypt.h |   7 ++
 5 files changed, 227 insertions(+), 212 deletions(-)
 create mode 100644 fs/ext4/ext4_crypto.c

--
2.31.1

Ritesh Harjani

author reviewer

2022-04-21 05:23 UTC #1 lore raw

RFC 1/6 fscrypt: Provide definition of fscrypt_set_test_dummy_encryption

Provide definition of fscrypt_set_test_dummy_encryption() when CONFIG_FS_ENCRYPTION is disabled. This then returns -EOPNOTSUPP.

Signed-off-by Ritesh Harjani <[email protected]>

---
 include/linux/fscrypt.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/fscrypt.h +7 -0

index 91ea9477e9bd..18dd6048bab3 100644
@@ -467,6 +467,13 @@ static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
 struct fscrypt_dummy_policy {
 };
 
+static inline int fscrypt_set_test_dummy_encryption(struct super_block *sb,
+				const char *arg,
+				struct fscrypt_dummy_policy *dummy_policy)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline void fscrypt_show_test_dummy_encryption(struct seq_file *seq,
 						      char sep,
 						      struct super_block *sb)

Signature · 1 line

2.31.1

Ritesh Harjani

author reviewer

2022-04-21 05:23 UTC #2 lore raw

RFC 2/6 ext4: Move ext4 crypto code to its own file ext4_crypto.c

In reply to Ritesh Harjani

This is to cleanup super.c file which has grown quite large.
So, start moving ext4 crypto related code to where it should be in the first place i.e. fs/ext4/ext4_crypto.c

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/Makefile      |   1 +
 fs/ext4/ext4.h        |   3 +
 fs/ext4/ext4_crypto.c | 127 ++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/super.c       | 122 ----------------------------------------
 4 files changed, 131 insertions(+), 122 deletions(-)
 create mode 100644 fs/ext4/ext4_crypto.c

fs/ext4/Makefile +1 -0

index 7d89142e1421..b340fe5f849c 100644
@@ -17,3 +17,4 @@ ext4-$(CONFIG_EXT4_FS_SECURITY)		+= xattr_security.o
 ext4-inode-test-objs			+= inode-test.o
 obj-$(CONFIG_EXT4_KUNIT_TESTS)		+= ext4-inode-test.o
 ext4-$(CONFIG_FS_VERITY)		+= verity.o
+ext4-$(CONFIG_FS_ENCRYPTION)		+= ext4_crypto.o

fs/ext4/ext4.h +3 -0

index 1d79012c5a5b..8bac8af25ed8 100644
@@ -2727,6 +2727,9 @@ extern int ext4_fname_setup_ci_filename(struct inode *dir,
 					 struct ext4_filename *fname);
 #endif
 
+/* ext4 encryption related stuff goes here ext4_crypto.c */
+extern const struct fscrypt_operations ext4_cryptops;
+
 #ifdef CONFIG_FS_ENCRYPTION
 static inline void ext4_fname_from_fscrypt_name(struct ext4_filename *dst,
 						const struct fscrypt_name *src)

fs/ext4/ext4_crypto.c new file +127 -0

127 lines not on this page · raw message

fs/ext4/super.c +0 ~~-122~~

128 lines not on this page · raw message

Signature · 1 line

2.31.1

Ritesh Harjani

author reviewer

2022-04-21 05:23 UTC #3 lore raw

RFC 3/6 ext4: Directly opencode ext4_set_test_dummy_encryption

In reply to Ritesh Harjani

Simplify the code by opencoding ext4_set_test_dummy_encryption(), since there is only one caller of it and ext4_** variant in itself does nothing special other than calling fscrypt_set_test_dummy_encryption(). This can be done directly by opencoding it in the caller.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/super.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

fs/ext4/super.c +14 ~~-20~~

index 8bb5fa15a013..e7e5c9c057d7 100644
@@ -1863,24 +1863,6 @@ ext4_sb_read_encoding(const struct ext4_super_block *es)
 }
 #endif
 
-static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
-{
-#ifdef CONFIG_FS_ENCRYPTION
-	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	int err;
-
-	err = fscrypt_set_test_dummy_encryption(sb, arg,
-						&sbi->s_dummy_enc_policy);
-	if (err) {
-		ext4_msg(sb, KERN_WARNING,
-			 "Error while setting test dummy encryption [%d]", err);
-		return err;
-	}
-	ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
-#endif
-	return 0;
-}
-
 #define EXT4_SPEC_JQUOTA			(1 <<  0)
 #define EXT4_SPEC_JQFMT				(1 <<  1)
 #define EXT4_SPEC_DATAJ				(1 <<  2)

@@ -2795,8 +2777,20 @@ static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
 
 	ext4_apply_quota_options(fc, sb);
 
-	if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)
-		ret = ext4_set_test_dummy_encryption(sb, ctx->test_dummy_enc_arg);
+	if (IS_ENABLED(CONFIG_FS_ENCRYPTION) &&
+			(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)) {
+
+		ret = fscrypt_set_test_dummy_encryption(sb,
+						ctx->test_dummy_enc_arg,
+						&sbi->s_dummy_enc_policy);
+		if (ret) {
+			ext4_msg(sb, KERN_WARNING, "Error while setting test dummy encryption [%d]",
+				 ret);
+			return ret;
+		}
+
+		ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
+	}
 
 	return ret;
 }

Signature · 1 line

2.31.1

Ritesh Harjani

author reviewer

2022-04-21 05:23 UTC #4 lore raw

RFC 4/6 ext4: Cleanup function defs from ext4.h into ext4_crypto.c

In reply to Ritesh Harjani

Some of these functions when CONFIG_FS_ENCRYPTION is enabled are not really inline (let compiler be the best judge of it).
Remove inline and move them into ext4_crypto.c where they should be present.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/ext4.h        | 70 ++++---------------------------------------
 fs/ext4/ext4_crypto.c | 65 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 64 deletions(-)

fs/ext4/ext4.h +6 ~~-64~~

index 8bac8af25ed8..caf154db4680 100644
@@ -2731,73 +2731,15 @@ extern int ext4_fname_setup_ci_filename(struct inode *dir,
 extern const struct fscrypt_operations ext4_cryptops;
 
 #ifdef CONFIG_FS_ENCRYPTION
-static inline void ext4_fname_from_fscrypt_name(struct ext4_filename *dst,
-						const struct fscrypt_name *src)
-{
-	memset(dst, 0, sizeof(*dst));
-
-	dst->usr_fname = src->usr_fname;
-	dst->disk_name = src->disk_name;
-	dst->hinfo.hash = src->hash;
-	dst->hinfo.minor_hash = src->minor_hash;
-	dst->crypto_buf = src->crypto_buf;
-}
-
-static inline int ext4_fname_setup_filename(struct inode *dir,
-					    const struct qstr *iname,
-					    int lookup,
-					    struct ext4_filename *fname)
-{
-	struct fscrypt_name name;
-	int err;
+int ext4_fname_setup_filename(struct inode *dir,
+			      const struct qstr *iname, int lookup,
+			      struct ext4_filename *fname);
 
-	err = fscrypt_setup_filename(dir, iname, lookup, &name);
-	if (err)
-		return err;
+int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
+			      struct ext4_filename *fname);
 
-	ext4_fname_from_fscrypt_name(fname, &name);
+void ext4_fname_free_filename(struct ext4_filename *fname);
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, iname, fname);
-#endif
-	return err;
-}
-
-static inline int ext4_fname_prepare_lookup(struct inode *dir,
-					    struct dentry *dentry,
-					    struct ext4_filename *fname)
-{
-	struct fscrypt_name name;
-	int err;
-
-	err = fscrypt_prepare_lookup(dir, dentry, &name);
-	if (err)
-		return err;
-
-	ext4_fname_from_fscrypt_name(fname, &name);
-
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
-#endif
-	return err;
-}
-
-static inline void ext4_fname_free_filename(struct ext4_filename *fname)
-{
-	struct fscrypt_name name;
-
-	name.crypto_buf = fname->crypto_buf;
-	fscrypt_free_filename(&name);
-
-	fname->crypto_buf.name = NULL;
-	fname->usr_fname = NULL;
-	fname->disk_name.name = NULL;
-
-#if IS_ENABLED(CONFIG_UNICODE)
-	kfree(fname->cf_name.name);
-	fname->cf_name.name = NULL;
-#endif
-}
 #else /* !CONFIG_FS_ENCRYPTION */
 static inline int ext4_fname_setup_filename(struct inode *dir,
 					    const struct qstr *iname,

fs/ext4/ext4_crypto.c +65 -0

71 lines not on this page · raw message

Signature · 1 line

2.31.1

Ritesh Harjani

author reviewer

2022-04-21 05:23 UTC #5 lore raw

RFC 5/6 ext4: Move all encryption related into a common #ifdef

In reply to Ritesh Harjani

This just moves left over usages of #ifdef CONFIG_FS_ENCRYPTION into a common place for function/macro definitions.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/ext4.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

fs/ext4/ext4.h +4 -6

index caf154db4680..2fb69c500063 100644
@@ -1440,12 +1440,6 @@ struct ext4_super_block {
 
 #ifdef __KERNEL__
 
-#ifdef CONFIG_FS_ENCRYPTION
-#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
-#else
-#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
-#endif
-
 /* Number of quota types we support */
 #define EXT4_MAXQUOTAS 3

@@ -2740,6 +2734,8 @@ int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
 
 void ext4_fname_free_filename(struct ext4_filename *fname);
 
+#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
+
 #else /* !CONFIG_FS_ENCRYPTION */
 static inline int ext4_fname_setup_filename(struct inode *dir,
 					    const struct qstr *iname,

@@ -2772,6 +2768,8 @@ static inline void ext4_fname_free_filename(struct ext4_filename *fname)
 	fname->cf_name.name = NULL;
 #endif
 }
+
+#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
 #endif /* !CONFIG_FS_ENCRYPTION */
 
 /* dir.c */

--
2.31.1

Ritesh Harjani

author reviewer

2022-04-21 05:24 UTC #6 lore raw

RFC 6/6 ext4: Use provided macro for checking dummy_enc_policy

In reply to Ritesh Harjani

We have a macro which test is dummy_enc_policy is enabled or not. Use that instead.

Signed-off-by Ritesh Harjani <[email protected]>

---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/ext4/super.c +1 -1

index e7e5c9c057d7..73fb54c3efd3 100644
@@ -2685,7 +2685,7 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
 	 * it to be specified during remount, but only if there is no change.
 	 */
 	if ((ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION) &&
-	    is_remount && !sbi->s_dummy_enc_policy.policy) {
+	    is_remount && !DUMMY_ENCRYPTION_ENABLED(sbi)) {
 		ext4_msg(NULL, KERN_WARNING,
 			 "Can't set test_dummy_encryption on remount");
 		return -1;

Signature · 1 line

2.31.1

Eric Biggers

maintainer

2022-04-21 07:24 UTC #7 lore raw

In reply to Ritesh Harjani

Quoted 19 lines · Ritesh Harjani

Hello,
This is 1st in the series to cleanup ext4/super.c, since it has grown quite large. This moves out crypto related ops and few definitions to fs/ext4/ext4_crypto.c
Testing
=========
1. Tested "-g encrypt" with default configs.
2. Compiled tested on x86 & Power.
Ritesh Harjani (6):
fscrypt: Provide definition of fscrypt_set_test_dummy_encryption
ext4: Move ext4 crypto code to its own file ext4_crypto.c
ext4: Directly opencode ext4_set_test_dummy_encryption
ext4: Cleanup function defs from ext4.h into ext4_crypto.c
ext4: Move all encryption related into a common #ifdef
ext4: Use provided macro for checking dummy_enc_policy

Quoted 7 lines

 fs/ext4/Makefile        |   1 +
 fs/ext4/ext4.h          |  81 +++--------------
 fs/ext4/ext4_crypto.c   | 192 ++++++++++++++++++++++++++++++++++++++++
 fs/ext4/super.c         | 158 ++++-----------------------------
 include/linux/fscrypt.h |   7 ++
 5 files changed, 227 insertions(+), 212 deletions(-)
 create mode 100644 fs/ext4/ext4_crypto.c

How about calling it crypto.c instead of ext4_crypto.c? It is already in the ext4 directory, so ext4 is implied.

Otherwise this patchset looks good to me.

Did you consider moving any of the other CONFIG_FS_ENCRYPTION code blocks into the new file as well? The implementation of FS_IOC_GET_ENCRYPTION_PWSALT might be a good candidate too.

- Eric

Ritesh Harjani

author reviewer

2022-04-21 15:07 UTC #8 lore raw

Quoted 28 lines · Eric Biggers

Quoted 19 lines · Ritesh Harjani
Hello,
This is 1st in the series to cleanup ext4/super.c, since it has grown quite large. This moves out crypto related ops and few definitions to fs/ext4/ext4_crypto.c
Testing
=========
1. Tested "-g encrypt" with default configs.
2. Compiled tested on x86 & Power.
Ritesh Harjani (6):
fscrypt: Provide definition of fscrypt_set_test_dummy_encryption
ext4: Move ext4 crypto code to its own file ext4_crypto.c
ext4: Directly opencode ext4_set_test_dummy_encryption
ext4: Cleanup function defs from ext4.h into ext4_crypto.c
ext4: Move all encryption related into a common #ifdef
ext4: Use provided macro for checking dummy_enc_policy
Quoted 7 lines
 fs/ext4/Makefile        |   1 +
 fs/ext4/ext4.h          |  81 +++--------------
 fs/ext4/ext4_crypto.c   | 192 ++++++++++++++++++++++++++++++++++++++++
 fs/ext4/super.c         | 158 ++++-----------------------------
 include/linux/fscrypt.h |   7 ++
 5 files changed, 227 insertions(+), 212 deletions(-)
 create mode 100644 fs/ext4/ext4_crypto.c

Quoted 2 lines

How about calling it crypto.c instead of ext4_crypto.c? It is already in the ext4 directory, so ext4 is implied.

Sure will do so in next revision which can also cover any other comments from others.

Quoted 2 lines

Otherwise this patchset looks good to me.

Sure thanks. May I add your Reviewed-by in next version if I don't see any major changes?

Quoted 4 lines

Did you consider moving any of the other CONFIG_FS_ENCRYPTION code blocks into the new file as well? The implementation of FS_IOC_GET_ENCRYPTION_PWSALT might be a good candidate too.

Sure, thanks for pointing out. I will take a look at it for next patch series or so.

-ritesh

Eric Biggers

maintainer

2022-05-01 07:18 UTC #9 lore raw

In reply to Ritesh Harjani

Quoted 11 lines · Ritesh Harjani

Hello,
This is 1st in the series to cleanup ext4/super.c, since it has grown quite large. This moves out crypto related ops and few definitions to fs/ext4/ext4_crypto.c
Testing
=========
1. Tested "-g encrypt" with default configs.
2. Compiled tested on x86 & Power.

Quoted 7 lines

Ritesh Harjani (6):
fscrypt: Provide definition of fscrypt_set_test_dummy_encryption
ext4: Move ext4 crypto code to its own file ext4_crypto.c
ext4: Directly opencode ext4_set_test_dummy_encryption
ext4: Cleanup function defs from ext4.h into ext4_crypto.c
ext4: Move all encryption related into a common #ifdef
ext4: Use provided macro for checking dummy_enc_policy

FYI, the patchset
https://lore.kernel.org/linux-ext4/[email protected]
I just sent out cleans up how the test_dummy_encryption mount option is handled. It would supersede patches 1, 3, 5, and 6 of this series (since those all only deal with test_dummy_encryption-related code).

To avoid conflicting changes, maybe you should just focus on your patches 2 and 4 for now, along with possibly FS_IOC_GET_ENCRYPTION_PWSALT as I mentioned? There shouldn't be any overlap that way.

- Eric

Ritesh Harjani

author reviewer

2022-05-02 14:26 UTC #10 lore raw

Quoted 20 lines · Eric Biggers

Quoted 11 lines · Ritesh Harjani
Hello,
This is 1st in the series to cleanup ext4/super.c, since it has grown quite large. This moves out crypto related ops and few definitions to fs/ext4/ext4_crypto.c
Testing
=========
1. Tested "-g encrypt" with default configs.
2. Compiled tested on x86 & Power.
Quoted 7 lines
Ritesh Harjani (6):
fscrypt: Provide definition of fscrypt_set_test_dummy_encryption
ext4: Move ext4 crypto code to its own file ext4_crypto.c
ext4: Directly opencode ext4_set_test_dummy_encryption
ext4: Cleanup function defs from ext4.h into ext4_crypto.c
ext4: Move all encryption related into a common #ifdef
ext4: Use provided macro for checking dummy_enc_policy

Quoted 5 lines

FYI, the patchset
https://lore.kernel.org/linux-ext4/[email protected]
I just sent out cleans up how the test_dummy_encryption mount option is handled. It would supersede patches 1, 3, 5, and 6 of this series (since those all only deal with test_dummy_encryption-related code).

Sure got it.

Quoted 4 lines

To avoid conflicting changes, maybe you should just focus on your patches 2 and 4 for now, along with possibly FS_IOC_GET_ENCRYPTION_PWSALT as I mentioned? There shouldn't be any overlap that way.

Yes, agreed with al of above points Eric. Will make the changes and send a new revision.

-ritesh