[00/25] fscrypt: add some higher-level helper functions

Commit Message

Dave Chinner Sept. 21, 2017, 6:45 a.m. UTC

On Wed, Sep 20, 2017 at 03:45:40PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> This series reduces code duplication among ext4, f2fs, and ubifs by
> introducing a S_ENCRYPTED inode flag (so we don't have to call back into
> the filesystem to test the filesystem-specific inode flag), then
> introducing new helper functions that are called at the beginning of the
> open, link, rename, lookup, and setattr operations.
> 
> In the future we maybe should even call these new helpers from the VFS
> so that each individual filesystem doesn't have to do it.  But that's
> not possible currently because fs/crypto/ can be built as a module.
> 
> Making changes like this is a bit challenging due to interdependencies
> between fscrypt and the individual filesystems, all of which have
> different maintainers.  For now my intent is that patches 1-10 be taken
> through the fscrypt tree --- though it's not perfect since patches 1-4
> do make some changes to each filesystem, as everyone must set
> S_ENCRYPTED before we can use it everywhere in the shared code.  But
> afterwards, patches 11-25 can be picked up by the individual filesystems
> to switch to the new helpers.

This all looks much nicer. Having just been looking at this stuff,
it makes the code much simpler to understand. So:

Acked-by: Dave Chinner <[e-mail]>

While I'm here, the fscrypt header file includes are clunky and
nasty. I worte a quick patch a couple of days ago to clean it up.
See below....

Cheers,

Dave.

Comments

Eric Biggers Sept. 21, 2017, 5:47 p.m. UTC | #1

Hi Dave,

On Thu, Sep 21, 2017 at 04:45:02PM +1000, Dave Chinner wrote:
> fscrypto: clean up include file mess
> 
> From: Dave Chinner <[e-mail]>
> 
> Filesystems have to include different header files based on whether
> they are compiled with encryption support or not. That's nasty and
> messy.
> 
> Instead, rationalise the headers so we have a single include
> fscrypt.h and let it decide what internal implementation to include
> based on the __FS_HAS_ENCRYPTION define. Filesystems set
> __FS_HAS_ENCRYPTION before including linux/fscrypt.h if they are
> built with encryption support.
> 
> Add guards to prevent fscrypt_supp.h and fscrypt_notsupp.h from
> being directly included by filesystems.

This looks good; we probably should have done it that way originally.  This will
allow us to have the inline functions like fscrypt_prepare_rename() defined in
fscrypt.h, and then have supp/notsupp versions of __fscrypt_prepare_rename()
instead --- so common checks like for IS_ENCRYPTED() will be in one place only.

One nit:

> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +#define __FS_HAS_ENCRYPTION 1
> +#endif
> +#include <linux/fscrypt.h>

How about doing

	#define __FS_HAS_ENCRYPTION IS_ENABLED(CONFIG_EXT4_FS_ENCRYPTION)

(and likewise for f2fs and ubifs), then checking '#if __FS_HAS_ENCRYPTION'
rather than '#ifdef __FS_HAS_ENCRYPTION'?

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner Sept. 21, 2017, 8:48 p.m. UTC | #2

On Thu, Sep 21, 2017 at 10:47:05AM -0700, Eric Biggers wrote:
> Hi Dave,
> 
> On Thu, Sep 21, 2017 at 04:45:02PM +1000, Dave Chinner wrote:
> > fscrypto: clean up include file mess
> > 
> > From: Dave Chinner <[e-mail]>
> > 
> > Filesystems have to include different header files based on whether
> > they are compiled with encryption support or not. That's nasty and
> > messy.
> > 
> > Instead, rationalise the headers so we have a single include
> > fscrypt.h and let it decide what internal implementation to include
> > based on the __FS_HAS_ENCRYPTION define. Filesystems set
> > __FS_HAS_ENCRYPTION before including linux/fscrypt.h if they are
> > built with encryption support.
> > 
> > Add guards to prevent fscrypt_supp.h and fscrypt_notsupp.h from
> > being directly included by filesystems.
> 
> This looks good; we probably should have done it that way originally.  This will
> allow us to have the inline functions like fscrypt_prepare_rename() defined in
> fscrypt.h, and then have supp/notsupp versions of __fscrypt_prepare_rename()
> instead --- so common checks like for IS_ENCRYPTED() will be in one place only.

*nod*

> One nit:
> 
> > +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> > +#define __FS_HAS_ENCRYPTION 1
> > +#endif
> > +#include <linux/fscrypt.h>
> 
> How about doing
> 
> 	#define __FS_HAS_ENCRYPTION IS_ENABLED(CONFIG_EXT4_FS_ENCRYPTION)
> 
> (and likewise for f2fs and ubifs), then checking '#if __FS_HAS_ENCRYPTION'
> rather than '#ifdef __FS_HAS_ENCRYPTION'?

Yeah, that's cleaner. I'll modify it and resend as a standalone
patch.

Cheers,

Dave.

Patch

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index a1d5021c31ef..a180981ee6d7 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -11,7 +11,8 @@ 
 #ifndef _FSCRYPT_PRIVATE_H
 #define _FSCRYPT_PRIVATE_H
 
-#include <linux/fscrypt_supp.h>
+#define __FS_HAS_ENCRYPTION 1
+#include <linux/fscrypt.h>
 #include <crypto/hash.h>
 
 /* Encryption parameters */
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index e2abe01c8c6b..900ac79879b3 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -33,17 +33,18 @@ 
 #include <linux/percpu_counter.h>
 #include <linux/ratelimit.h>
 #include <crypto/hash.h>
-#ifdef CONFIG_EXT4_FS_ENCRYPTION
-#include <linux/fscrypt_supp.h>
-#else
-#include <linux/fscrypt_notsupp.h>
-#endif
+
 #include <linux/falloc.h>
 #include <linux/percpu-rwsem.h>
 #ifdef __KERNEL__
 #include <linux/compat.h>
 #endif
 
+#ifdef CONFIG_EXT4_FS_ENCRYPTION
+#define __FS_HAS_ENCRYPTION 1
+#endif
+#include <linux/fscrypt.h>
+
 /*
  * The fourth extended filesystem constants/structures
  */
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9a7c90386947..66502c5f7d67 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -23,12 +23,12 @@ 
 #include <linux/bio.h>
 #include <linux/blkdev.h>
 #include <linux/quotaops.h>
+#include <crypto/hash.h>
+
 #ifdef CONFIG_F2FS_FS_ENCRYPTION
-#include <linux/fscrypt_supp.h>
-#else
-#include <linux/fscrypt_notsupp.h>
+#define __FS_HAS_ENCRYPTION 1
 #endif
-#include <crypto/hash.h>
+#include <linux/fscrypt.h>
 
 #ifdef CONFIG_F2FS_CHECK_FS
 #define f2fs_bug_on(sbi, condition)	BUG_ON(condition)
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index cd43651f1731..e5b6c8f02133 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -38,12 +38,13 @@ 
 #include <linux/backing-dev.h>
 #include <linux/security.h>
 #include <linux/xattr.h>
+#include <linux/random.h>
+
 #ifdef CONFIG_UBIFS_FS_ENCRYPTION
-#include <linux/fscrypt_supp.h>
-#else
-#include <linux/fscrypt_notsupp.h>
+#define __FS_HAS_ENCRYPTION 1
 #endif
-#include <linux/random.h>
+#include <linux/fscrypt.h>
+
 #include "ubifs-media.h"
 
 /* Version of this UBIFS implementation */
diff --git a/include/linux/fscrypt_common.h b/include/linux/fscrypt.h
similarity index 79%
rename from include/linux/fscrypt_common.h
rename to include/linux/fscrypt.h
index 97f738628b36..4db0a7ec26d9 100644
--- a/include/linux/fscrypt_common.h
+++ b/include/linux/fscrypt.h
@@ -1,14 +1,17 @@ 
 /*
- * fscrypt_common.h: common declarations for per-file encryption
+ * fscrypt.h: declarations for per-file encryption
+ *
+ * Filesystems that implement per-file encryption include this header
+ * file with the __FS_HAS_ENCRYPTION set according to whether that filesystem
+ * is being built with encryption support or not.
  *
  * Copyright (C) 2015, Google, Inc.
  *
  * Written by Michael Halcrow, 2015.
  * Modified by Jaegeuk Kim, 2015.
  */
-
-#ifndef _LINUX_FSCRYPT_COMMON_H
-#define _LINUX_FSCRYPT_COMMON_H
+#ifndef _LINUX_FSCRYPT_H
+#define _LINUX_FSCRYPT_H
 
 #include <linux/key.h>
 #include <linux/fs.h>
@@ -119,23 +122,35 @@  static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
 	return false;
 }
 
+#ifdef __FS_HAS_ENCRYPTION
+
 static inline struct page *fscrypt_control_page(struct page *page)
 {
-#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
 	return ((struct fscrypt_ctx *)page_private(page))->w.control_page;
-#else
+}
+
+static inline bool fscrypt_has_encryption_key(const struct inode *inode)
+{
+	return (inode->i_crypt_info != NULL);
+}
+
+#include <linux/fscrypt_supp.h>
+
+#else /* !__FS_HAS_ENCRYPTION */
+
+static inline struct page *fscrypt_control_page(struct page *page)
+{
 	WARN_ON_ONCE(1);
 	return ERR_PTR(-EINVAL);
-#endif
 }
 
-static inline int fscrypt_has_encryption_key(const struct inode *inode)
+static inline bool fscrypt_has_encryption_key(const struct inode *inode)
 {
-#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
-	return (inode->i_crypt_info != NULL);
-#else
 	return 0;
-#endif
 }
 
-#endif	/* _LINUX_FSCRYPT_COMMON_H */
+#include <linux/fscrypt_notsupp.h>
+#endif	/* __FS_HAS_ENCRYPTION */
+
+
+#endif	/* _LINUX_FSCRYPT_H */
diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
index ec406aed2f2f..2d0b6960831e 100644
--- a/include/linux/fscrypt_notsupp.h
+++ b/include/linux/fscrypt_notsupp.h
@@ -3,13 +3,16 @@ 
  *
  * This stubs out the fscrypt functions for filesystems configured without
  * encryption support.
+ *
+ * Do not include this file directly. Use fscrypt.h instead!
  */
+#ifndef _LINUX_FSCRYPT_H
+#error "Incorrect include of linux/fscrypt_notsupp.h!"
+#endif
 
 #ifndef _LINUX_FSCRYPT_NOTSUPP_H
 #define _LINUX_FSCRYPT_NOTSUPP_H
 
-#include <linux/fscrypt_common.h>
-
 /* crypto.c */
 static inline struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *inode,
 						  gfp_t gfp_flags)
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index 32e2fcf13b01..5a90e5ef4687 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -1,14 +1,15 @@ 
 /*
  * fscrypt_supp.h
  *
- * This is included by filesystems configured with encryption support.
+ * Do not include this file directly. Use fscrypt.h instead!
  */
+#ifndef _LINUX_FSCRYPT_H
+#error "Incorrect include of linux/fscrypt_supp.h!"
+#endif
 
 #ifndef _LINUX_FSCRYPT_SUPP_H
 #define _LINUX_FSCRYPT_SUPP_H
 
-#include <linux/fscrypt_common.h>
-
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);