Skip to contentSkip to conversation

ext4/crypto: Move out crypto related ops to crypto.c

PATCH Superseded Ritesh Harjani 2 participants 10 messages May 14, 2022 – May 15, 2022

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Ritesh Harjani

author reviewer
#0 lore raw

v2 cover ext4/crypto: Move out crypto related ops to 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 fs encryption related definitions to fs/ext4/crypto.c

I have tested "-g encrypt" xfstests group and don't see any surprises there. The changes are relatively straight forward since it is just moving/refactoring.

Currently these patches apply cleanly on ext4 tree's dev branch. Since in these series test_dummy_encryption related changes are dropped, hence I don't think that this should give any major conflict with Eric's series.

NOTE: I noticed we could move both ext4 & f2fs to use uuid_t lib API from include/linux/uuid.h for managing sb->s_encrypt_pw_salt.
That should kill custom implementations of uuid_is_zero()/uuid_is_nonzero(). But since I noticed it while writing this cover letter, so I would like to do those changes in a seperate patch series if that is ok. That way maybe we could cleanup tree wide changes in fs/ (if there are others too).

RFC -> v2
==========
1. Dropped all test_dummy_encryption related changes
(Eric has separately submitted a v3 for fixing more general problems with
that mount option).
2. Addressed Eric comments to:-
1. rename ext4_crypto.c -> crypto.c
2. Refactor out ext4_ioc_get_encryption_pwsalt() into crypto.c
3. Made ext4_fname_from_fscrypt_name() static since it is only being called from within crypto.c functions.

[RFC] - https://lore.kernel.org/linux-ext4/[email protected]/

Ritesh Harjani (3):
ext4: Move ext4 crypto code to its own file crypto.c
ext4: Cleanup function defs from ext4.h into crypto.c
ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()

 fs/ext4/Makefile |   1 +
 fs/ext4/crypto.c | 245 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h   |  79 ++++-----------
 fs/ext4/ioctl.c  |  58 +----------
 fs/ext4/super.c  | 122 -----------------------
 5 files changed, 264 insertions(+), 241 deletions(-)
 create mode 100644 fs/ext4/crypto.c

--
2.31.1

author reviewer
#1 lore raw

PATCH v2 1/3 ext4: Move ext4 crypto code to its own file crypto.c

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/crypto.c

---
 fs/ext4/Makefile |   1 +
 fs/ext4/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h   |   3 ++
 fs/ext4/super.c  | 122 ---------------------------------------------
 4 files changed, 131 insertions(+), 122 deletions(-)
 create mode 100644 fs/ext4/crypto.c
fs/ext4/Makefile +1 -0
index 7d89142e1421..72206a292676 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)		+= crypto.o
fs/ext4/crypto.c new file +127 -0

127 lines not on this page · raw message

fs/ext4/ext4.h +3 -0
index a743b1e3b89e..9100f0ba4a52 100644
@@ -2731,6 +2731,9 @@ extern int ext4_fname_setup_ci_filename(struct inode *dir,
 					 struct ext4_filename *fname);
 #endif
 
+/* ext4 encryption related stuff goes here 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/super.c +0 -122

128 lines not on this page · raw message

Signature · 1 line
2.31.1
author reviewer
#4 lore raw

PATCH v2 2/3 ext4: Cleanup function defs from ext4.h into 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 crypto.c where they should be present.

---
 fs/ext4/crypto.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h   | 70 +++++-------------------------------------------
 2 files changed, 71 insertions(+), 64 deletions(-)
fs/ext4/crypto.c +65 -0
index e5413c0970ee..f8333927f0f6 100644
@@ -6,6 +6,71 @@
 #include "xattr.h"
 #include "ext4_jbd2.h"
 
+static 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;
+}
+
+int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
+			      int lookup, struct ext4_filename *fname)
+{
+	struct fscrypt_name name;
+	int err;
+
+	err = fscrypt_setup_filename(dir, iname, lookup, &name);
+	if (err)
+		return err;
+
+	ext4_fname_from_fscrypt_name(fname, &name);
+
+#if IS_ENABLED(CONFIG_UNICODE)
+	err = ext4_fname_setup_ci_filename(dir, iname, fname);
+#endif
+	return err;
+}
+
+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;
+}
+
+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
+}
+
 static int ext4_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	return ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
fs/ext4/ext4.h +6 -64

79 lines not on this page · raw message

Signature · 1 line
2.31.1
author reviewer
#7 lore raw

PATCH v2 3/3 ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()

In reply to Ritesh Harjani

This patch move code for FS_IOC_GET_ENCRYPTION_PWSALT case into ext4's crypto.c file, i.e. ext4_ioc_get_encryption_pwsalt() and uuid_is_zero(). This is mostly refactoring logic and should not affect any functionality change.

---
 fs/ext4/crypto.c | 53 +++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h   |  8 +++++++
 fs/ext4/ioctl.c  | 58 ++----------------------------------------------
 3 files changed, 63 insertions(+), 56 deletions(-)
fs/ext4/crypto.c +53 -0
index f8333927f0f6..36bcfeecdb00 100644
@@ -71,6 +71,59 @@ void ext4_fname_free_filename(struct ext4_filename *fname)
 #endif
 }
 
+static bool uuid_is_zero(__u8 u[16])
+{
+	int i;
+
+	for (i = 0; i < 16; i++)
+		if (u[i])
+			return false;
+	return true;
+}
+
+int ext4_ioc_get_encryption_pwsalt(struct file *filp, void __user *arg)
+{
+	struct super_block *sb = file_inode(filp)->i_sb;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	int err, err2;
+	handle_t *handle;
+
+	if (!ext4_has_feature_encrypt(sb))
+		return -EOPNOTSUPP;
+
+	if (uuid_is_zero(sbi->s_es->s_encrypt_pw_salt)) {
+		err = mnt_want_write_file(filp);
+		if (err)
+			return err;
+		handle = ext4_journal_start_sb(sb, EXT4_HT_MISC, 1);
+		if (IS_ERR(handle)) {
+			err = PTR_ERR(handle);
+			goto pwsalt_err_exit;
+		}
+		err = ext4_journal_get_write_access(handle, sb, sbi->s_sbh,
+						    EXT4_JTR_NONE);
+		if (err)
+			goto pwsalt_err_journal;
+		lock_buffer(sbi->s_sbh);
+		generate_random_uuid(sbi->s_es->s_encrypt_pw_salt);
+		ext4_superblock_csum_set(sb);
+		unlock_buffer(sbi->s_sbh);
+		err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
+pwsalt_err_journal:
+		err2 = ext4_journal_stop(handle);
+		if (err2 && !err)
+			err = err2;
+pwsalt_err_exit:
+		mnt_drop_write_file(filp);
+		if (err)
+			return err;
+	}
+
+	if (copy_to_user(arg, sbi->s_es->s_encrypt_pw_salt, 16))
+		return -EFAULT;
+	return 0;
+}
+
 static int ext4_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	return ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
fs/ext4/ext4.h +8 -0
index 11bb0d2ee2eb..b61115fe7ffb 100644
@@ -2744,6 +2744,8 @@ int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
 
 void ext4_fname_free_filename(struct ext4_filename *fname);
 
+int ext4_ioc_get_encryption_pwsalt(struct file *filp, void __user *arg);
+
 #else /* !CONFIG_FS_ENCRYPTION */
 static inline int ext4_fname_setup_filename(struct inode *dir,
 					    const struct qstr *iname,
@@ -2776,6 +2778,12 @@ static inline void ext4_fname_free_filename(struct ext4_filename *fname)
 	fname->cf_name.name = NULL;
 #endif
 }
+
+static inline int ext4_ioc_get_encryption_pwsalt(struct file *filp,
+						 void __user *arg)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* !CONFIG_FS_ENCRYPTION */
 
 /* dir.c */
fs/ext4/ioctl.c +2 -56

71 lines not on this page · raw message

Signature · 1 line
2.31.1
maintainer Reviewed-by on v2 1/3
#2 lore raw

Re: [PATCHv2 1/3] ext4: Move ext4 crypto code to its own file crypto.c

In reply to Ritesh Harjani

fs/ext4/ext4.h extern int ext4_fname_setup_ci_filename(struct inode *dir, · line 2737

Quoted 12 lines · Ritesh Harjani
fs/ext4/ext4.h +3 -0
index a743b1e3b89e..9100f0ba4a52 100644
@@ -2731,6 +2731,9 @@ extern int ext4_fname_setup_ci_filename(struct inode *dir,
 					 struct ext4_filename *fname);
 #endif
 
+/* ext4 encryption related stuff goes here crypto.c */
+extern const struct fscrypt_operations ext4_cryptops;
+
 #ifdef CONFIG_FS_ENCRYPTION

Shouldn't the declaration of ext4_cryptops go in the CONFIG_FS_ENCRYPTION block?

Otherwise this patch looks good, thanks.

- Eric

maintainer Reviewed-by on v2 2/3
#5 lore raw

Re: [PATCHv2 2/3] ext4: Cleanup function defs from ext4.h into crypto.c

In reply to Ritesh Harjani
Quoted 1 line · Ritesh Harjani
fs/ext4/crypto.c +0 -0

[...]

Quoted 3 lines
fs/ext4/crypto.c +3 -0
+int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
+			      int lookup, struct ext4_filename *fname)
+{

[...]

Quoted 1 line
fs/ext4/ext4.h +0 -0

[...]

fs/ext4/ext4.h extern int ext4_fname_setup_ci_filename(struct inode *dir, · line 2740

Quoted 3 lines
fs/ext4/ext4.h +3 -0
+int ext4_fname_setup_filename(struct inode *dir,
+			      const struct qstr *iname, int lookup,
+			      struct ext4_filename *fname);

Very minor nit: the above declaration can be formatted on 2 lines, the same as the definition.

Otherwise this patch looks fine. I think that filename handling in ext4 in general is still greatly in need of some cleanups, considering that ext4 now has to support all combinations of encryption and casefolding. f2fs does it in a somewhat cleaner way, IMO. And it's possible that would lead us down a slightly different path. But this is an improvement for now.

- Eric

maintainer Reviewed-by on v2 3/3
#8 lore raw

Re: [PATCHv2 3/3] ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()

In reply to Ritesh Harjani

fs/ext4/ioctl.c

Quoted 1 line · Ritesh Harjani
fs/ext4/ioctl.c +0 -0

The include <linux/uuid.h> can be removed from this file.

Quoted 1 line
fs/ext4/crypto.c +0 -0

[...]

Quoted 1 line

+int ext4_ioc_get_encryption_pwsalt(struct file *filp, void __user *arg)

ext4 has more functions named "ext4_ioctl_*" thtan "ext4_ioc_*", so it might be worth adding those extra 2 letters for consistency.

Other than the above minor nits this looks good, thanks!

- Eric

author reviewer
#3 lore raw

Re: [PATCHv2 1/3] ext4: Move ext4 crypto code to its own file crypto.c

In reply to Eric Biggers
Quoted 9 lines · Eric Biggers
Quoted 8 lines · Ritesh Harjani
fs/ext4/ext4.h +0 -0
index a743b1e3b89e..9100f0ba4a52 100644
@@ -2731,6 +2731,9 @@ extern int ext4_fname_setup_ci_filename(struct inode *dir,
 					 struct ext4_filename *fname);
 #endif
 
Quoted 6 lines

fs/ext4/ext4.h extern int ext4_fname_setup_ci_filename(struct inode *dir, · line 2737

Quoted 4 lines
fs/ext4/ext4.h +3 -0
+/* ext4 encryption related stuff goes here crypto.c */
+extern const struct fscrypt_operations ext4_cryptops;
+
 #ifdef CONFIG_FS_ENCRYPTION

Shouldn't the declaration of ext4_cryptops go in the CONFIG_FS_ENCRYPTION block?

Sure yes. I should move that within CONFIG_FS_ENCRYPTION block.

Quoted 4 lines

Otherwise this patch looks good, thanks.

Thanks for the review.

-ritesh

author reviewer
#6 lore raw

Re: [PATCHv2 2/3] ext4: Cleanup function defs from ext4.h into crypto.c

In reply to Eric Biggers
Quoted 7 lines · Eric Biggers
Quoted 1 line · Ritesh Harjani
fs/ext4/crypto.c +0 -0

[...]

Quoted 3 lines
fs/ext4/crypto.c +3 -0
+int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
+			      int lookup, struct ext4_filename *fname)
+{

[...]

Quoted 8 lines
Quoted 1 line
fs/ext4/ext4.h +0 -0

[...]

fs/ext4/ext4.h extern int ext4_fname_setup_ci_filename(struct inode *dir, · line 2740

Quoted 3 lines
fs/ext4/ext4.h +3 -0
+int ext4_fname_setup_filename(struct inode *dir,
+			      const struct qstr *iname, int lookup,
+			      struct ext4_filename *fname);

Very minor nit: the above declaration can be formatted on 2 lines, the same as the definition.

Thanks for spotting. I will make the change.

Quoted 6 lines

Otherwise this patch looks fine. I think that filename handling in ext4 in general is still greatly in need of some cleanups, considering that ext4 now has to support all combinations of encryption and casefolding. f2fs does it in a somewhat cleaner way, IMO. And it's possible that would lead us down a slightly different path. But this is an improvement for now.

Some examples please which you posibly have in mind which should help in cleanup ext4's filename handling code. I can get back to it after completing some other items in my todo list.

Quoted 2 lines

Thanks!!

-ritesh

author reviewer
#9 lore raw

Re: [PATCHv2 3/3] ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()

In reply to Eric Biggers
Quoted 4 lines · Eric Biggers

fs/ext4/ioctl.c

Quoted 1 line · Ritesh Harjani
fs/ext4/ioctl.c +0 -0

The include <linux/uuid.h> can be removed from this file.

Yes. I will remove this from ioctl.c and will add it to crypto.c for generate_random_uuid() definition.

Quoted 7 lines
Quoted 2 lines
fs/ext4/crypto.c +0 -0

[...]

Quoted 1 line

+int ext4_ioc_get_encryption_pwsalt(struct file *filp, void __user *arg)

ext4 has more functions named "ext4_ioctl_*" thtan "ext4_ioc_*", so it might be worth adding those extra 2 letters for consistency.

You are right.

Quoted 4 lines

Other than the above minor nits this looks good, thanks!

Thanks for a thorough review.

I will make these changes and send out a v3 soon.

-ritesh