diff mbox series

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

Message ID 3256b969d6e858414f08e0ca2f5117e76fdc2057.1652539361.git.ritesh.list@gmail.com (mailing list archive)
State Superseded
Headers show
Series ext4/crypto: Move out crypto related ops to crypto.c | expand

Commit Message

Ritesh Harjani (IBM) May 14, 2022, 5:22 p.m. UTC 
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.

Suggested-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Ritesh Harjani <ritesh.list@gmail.com>
---
 fs/ext4/crypto.c | 53 +++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h   |  8 +++++++
 fs/ext4/ioctl.c  | 58 ++----------------------------------------------
 3 files changed, 63 insertions(+), 56 deletions(-)

Comments

Eric Biggers May 15, 2022, 3:42 a.m. UTC | #1 
On Sat, May 14, 2022 at 10:52:48PM +0530, Ritesh Harjani wrote:
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c

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

> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
[...]
> +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!

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric
Ritesh Harjani (IBM) May 15, 2022, 4:51 a.m. UTC | #2 
On 22/05/14 08:42PM, Eric Biggers wrote:
> On Sat, May 14, 2022 at 10:52:48PM +0530, Ritesh Harjani wrote:
> > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>
> 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.

>
> > diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> [...]
> > +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.

>
> Other than the above minor nits this looks good, thanks!
>
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Thanks for a thorough review.

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

-ritesh
diff mbox series

Patch

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index f8333927f0f6..36bcfeecdb00 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -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,
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 11bb0d2ee2eb..b61115fe7ffb 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -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 */
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index ba44fa1be70a..044e65d44054 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -504,18 +504,6 @@  static long swap_inode_boot_loader(struct super_block *sb,
 	return err;
 }
 
-#ifdef CONFIG_FS_ENCRYPTION
-static int uuid_is_zero(__u8 u[16])
-{
-	int	i;
-
-	for (i = 0; i < 16; i++)
-		if (u[i])
-			return 0;
-	return 1;
-}
-#endif
-
 /*
  * If immutable is set and we are not clearing it, we're not allowed to change
  * anything else in the inode.  Don't error out if we're only trying to set
@@ -1432,51 +1420,9 @@  static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			return -EOPNOTSUPP;
 		return fscrypt_ioctl_set_policy(filp, (const void __user *)arg);
 
-	case FS_IOC_GET_ENCRYPTION_PWSALT: {
-#ifdef CONFIG_FS_ENCRYPTION
-		int err, err2;
-		struct ext4_sb_info *sbi = EXT4_SB(sb);
-		handle_t *handle;
+	case FS_IOC_GET_ENCRYPTION_PWSALT:
+		return ext4_ioc_get_encryption_pwsalt(filp, (void __user *)arg);
 
-		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((void __user *) arg,
-				 sbi->s_es->s_encrypt_pw_salt, 16))
-			return -EFAULT;
-		return 0;
-#else
-		return -EOPNOTSUPP;
-#endif
-	}
 	case FS_IOC_GET_ENCRYPTION_POLICY:
 		if (!ext4_has_feature_encrypt(sb))
 			return -EOPNOTSUPP;