ext4/crypto: Move out crypto related ops to crypto.c
Ritesh Harjani
reviewerv2 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
Ritesh Harjani
reviewerPATCH 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
@@ -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
@@ -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
Ritesh Harjani
reviewerPATCH v2 2/3 ext4: Cleanup function defs from ext4.h into crypto.c
In reply to Ritesh HarjaniSome 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
@@ -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
Ritesh Harjani
reviewerPATCH v2 3/3 ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()
In reply to Ritesh HarjaniThis 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
@@ -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
@@ -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
Re: [PATCHv2 1/3] ext4: Move ext4 crypto code to its own file crypto.c
In reply to Ritesh Harjanifs/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@@ -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
Re: [PATCHv2 2/3] ext4: Cleanup function defs from ext4.h into crypto.c
In reply to Ritesh HarjaniQuoted 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
Re: [PATCHv2 3/3] ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()
In reply to Ritesh Harjanifs/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
Ritesh Harjani
reviewerRe: [PATCHv2 1/3] ext4: Move ext4 crypto code to its own file crypto.c
In reply to Eric BiggersQuoted 9 lines · Eric Biggers
Quoted 8 lines · Ritesh Harjani
fs/ext4/ext4.h +0
-0@@ -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_cryptopsgo in theCONFIG_FS_ENCRYPTIONblock?
Sure yes. I should move that within CONFIG_FS_ENCRYPTION block.
Quoted 4 lines
Otherwise this patch looks good, thanks.
Reviewed-by Eric Biggers <[email protected]>
Thanks for the review.
-ritesh
Ritesh Harjani
reviewerRe: [PATCHv2 2/3] ext4: Cleanup function defs from ext4.h into crypto.c
In reply to Eric BiggersQuoted 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
Reviewed-by Eric Biggers <[email protected]>
Thanks!!
-ritesh
Ritesh Harjani
reviewerRe: [PATCHv2 3/3] ext4: Refactor and move ext4_ioc_get_encryption_pwsalt()
In reply to Eric BiggersQuoted 4 lines · Eric Biggers
fs/ext4/ioctl.c
Quoted 1 line · Ritesh Harjani
fs/ext4/ioctl.c +0
-0The 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!
Reviewed-by Eric Biggers <[email protected]>
Thanks for a thorough review.
I will make these changes and send out a v3 soon.
-ritesh