[PATCH 0/2] ecryptfs: convert to the new mount API

[PATCH 0/2] ecryptfs: convert to the new mount API

[Eric Sandeen]

This is lightly tested with the kernel tests present in ecryptfs-utils,
but it could certainly use a bit more testing and review, particularly
with invalid mount option sets.

This one is a little unique compared to other filesystems in that I
allocate both an fs context and the *sbi in .init_fs_context; the *sbi
is long-lived, and the context is only present during the initial mount.

Allocating sbi with the filesystem context means we can set options
into it directly, rather than needing to do it after parsing. And it's
particularly simple to do it this way given that there is no remount.

I could squash these two patches into one if you prefer, but
I thought maybe breaking out the first change made review a little
easier.

Thanks,
-Eric

[PATCH 1/2] ecryptfs: Factor out mount option validation

[Eric Sandeen]

Under the new mount API, mount options are parsed one at a time.
Any validation that examines multiple options must be done after parsing
is complete, so factor out a ecryptfs_validate_options() which can be
called separately.

To facilitate this, temporarily move the local variables that tracked
whether various options have been set in the parsing function, into the
ecryptfs_mount_crypt_stat structure so that they can be examined later.

These will be moved to a more ephemeral struct in the mount api conversion
patch to follow.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/ecryptfs/ecryptfs_kernel.h |  7 +++++
 fs/ecryptfs/main.c            | 55 ++++++++++++++++++++---------------
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index c586c5db18b5..d359ec085a70 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -343,6 +343,13 @@ struct ecryptfs_mount_crypt_stat {
 	unsigned char global_default_fn_cipher_name[
 		ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
 	char global_default_fnek_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
+	/* Mount option status trackers */
+	bool check_ruid;
+	bool sig_set;
+	bool cipher_name_set;
+	bool cipher_key_bytes_set;
+	bool fn_cipher_name_set;
+	bool fn_cipher_key_bytes_set;
 };
 
 /* superblock private data. */
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index 577c56302314..d03f1c6ccc1c 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -239,18 +239,12 @@ static void ecryptfs_init_mount_crypt_stat(
  *
  * Returns zero on success; non-zero on error
  */
-static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
-				  uid_t *check_ruid)
+static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options)
 {
 	char *p;
 	int rc = 0;
-	int sig_set = 0;
-	int cipher_name_set = 0;
-	int fn_cipher_name_set = 0;
 	int cipher_key_bytes;
-	int cipher_key_bytes_set = 0;
 	int fn_cipher_key_bytes;
-	int fn_cipher_key_bytes_set = 0;
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
 		&sbi->mount_crypt_stat;
 	substring_t args[MAX_OPT_ARGS];
@@ -261,9 +255,6 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 	char *fnek_src;
 	char *cipher_key_bytes_src;
 	char *fn_cipher_key_bytes_src;
-	u8 cipher_code;
-
-	*check_ruid = 0;
 
 	if (!options) {
 		rc = -EINVAL;
@@ -285,14 +276,14 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 				       "global sig; rc = [%d]\n", rc);
 				goto out;
 			}
-			sig_set = 1;
+			mount_crypt_stat->sig_set = 1;
 			break;
 		case ecryptfs_opt_cipher:
 		case ecryptfs_opt_ecryptfs_cipher:
 			cipher_name_src = args[0].from;
 			strscpy(mount_crypt_stat->global_default_cipher_name,
 				cipher_name_src);
-			cipher_name_set = 1;
+			mount_crypt_stat->cipher_name_set = 1;
 			break;
 		case ecryptfs_opt_ecryptfs_key_bytes:
 			cipher_key_bytes_src = args[0].from;
@@ -301,7 +292,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 						   &cipher_key_bytes_src, 0);
 			mount_crypt_stat->global_default_cipher_key_size =
 				cipher_key_bytes;
-			cipher_key_bytes_set = 1;
+			mount_crypt_stat->cipher_key_bytes_set = 1;
 			break;
 		case ecryptfs_opt_passthrough:
 			mount_crypt_stat->flags |=
@@ -340,7 +331,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 			fn_cipher_name_src = args[0].from;
 			strscpy(mount_crypt_stat->global_default_fn_cipher_name,
 				fn_cipher_name_src);
-			fn_cipher_name_set = 1;
+			mount_crypt_stat->fn_cipher_name_set = 1;
 			break;
 		case ecryptfs_opt_fn_cipher_key_bytes:
 			fn_cipher_key_bytes_src = args[0].from;
@@ -349,7 +340,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 						   &fn_cipher_key_bytes_src, 0);
 			mount_crypt_stat->global_default_fn_cipher_key_bytes =
 				fn_cipher_key_bytes;
-			fn_cipher_key_bytes_set = 1;
+			mount_crypt_stat->fn_cipher_key_bytes_set = 1;
 			break;
 		case ecryptfs_opt_unlink_sigs:
 			mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
@@ -359,7 +350,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 				ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
 			break;
 		case ecryptfs_opt_check_dev_ruid:
-			*check_ruid = 1;
+			mount_crypt_stat->check_ruid = 1;
 			break;
 		case ecryptfs_opt_err:
 		default:
@@ -368,14 +359,25 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 			       __func__, p);
 		}
 	}
-	if (!sig_set) {
+
+out:
+	return rc;
+}
+
+static int ecryptfs_validate_options(
+		struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
+{
+	int rc = 0;
+	u8 cipher_code;
+
+	if (!mount_crypt_stat->sig_set) {
 		rc = -EINVAL;
 		ecryptfs_printk(KERN_ERR, "You must supply at least one valid "
 				"auth tok signature as a mount "
 				"parameter; see the eCryptfs README\n");
 		goto out;
 	}
-	if (!cipher_name_set) {
+	if (!mount_crypt_stat->cipher_name_set) {
 		int cipher_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER);
 
 		BUG_ON(cipher_name_len > ECRYPTFS_MAX_CIPHER_NAME_SIZE);
@@ -383,13 +385,13 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 		       ECRYPTFS_DEFAULT_CIPHER);
 	}
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
-	    && !fn_cipher_name_set)
+	    && !mount_crypt_stat->fn_cipher_name_set)
 		strcpy(mount_crypt_stat->global_default_fn_cipher_name,
 		       mount_crypt_stat->global_default_cipher_name);
-	if (!cipher_key_bytes_set)
+	if (!mount_crypt_stat->cipher_key_bytes_set)
 		mount_crypt_stat->global_default_cipher_key_size = 0;
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
-	    && !fn_cipher_key_bytes_set)
+	    && !mount_crypt_stat->fn_cipher_key_bytes_set)
 		mount_crypt_stat->global_default_fn_cipher_key_bytes =
 			mount_crypt_stat->global_default_cipher_key_size;
 
@@ -469,7 +471,6 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	const char *err = "Getting sb failed";
 	struct inode *inode;
 	struct path path;
-	uid_t check_ruid;
 	int rc;
 
 	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
@@ -484,12 +485,17 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 		goto out;
 	}
 
-	rc = ecryptfs_parse_options(sbi, raw_data, &check_ruid);
+	rc = ecryptfs_parse_options(sbi, raw_data);
 	if (rc) {
 		err = "Error parsing options";
 		goto out;
 	}
 	mount_crypt_stat = &sbi->mount_crypt_stat;
+	rc = ecryptfs_validate_options(mount_crypt_stat);
+	if (rc) {
+		err = "Error validationg options";
+		goto out;
+	}
 
 	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
 	if (IS_ERR(s)) {
@@ -529,7 +535,8 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 		goto out_free;
 	}
 
-	if (check_ruid && !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
+	if (mount_crypt_stat->check_ruid &&
+	    !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
 		rc = -EPERM;
 		printk(KERN_ERR "Mount of device (uid: %d) not owned by "
 		       "requested user (uid: %d)\n",
-- 
2.46.0

[PATCH 2/2] ecryptfs: Convert ecryptfs to use the new mount API

[Eric Sandeen]

Convert ecryptfs to the new mount API.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/ecryptfs/ecryptfs_kernel.h |   7 -
 fs/ecryptfs/main.c            | 393 +++++++++++++++++-----------------
 2 files changed, 198 insertions(+), 202 deletions(-)

diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index d359ec085a70..c586c5db18b5 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -343,13 +343,6 @@ struct ecryptfs_mount_crypt_stat {
 	unsigned char global_default_fn_cipher_name[
 		ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
 	char global_default_fnek_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
-	/* Mount option status trackers */
-	bool check_ruid;
-	bool sig_set;
-	bool cipher_name_set;
-	bool cipher_key_bytes_set;
-	bool fn_cipher_name_set;
-	bool fn_cipher_key_bytes_set;
 };
 
 /* superblock private data. */
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index d03f1c6ccc1c..a7829983304e 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -15,10 +15,10 @@
 #include <linux/module.h>
 #include <linux/namei.h>
 #include <linux/skbuff.h>
-#include <linux/mount.h>
 #include <linux/pagemap.h>
 #include <linux/key.h>
-#include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include <linux/fs_stack.h>
 #include <linux/slab.h>
 #include <linux/magic.h>
@@ -153,32 +153,31 @@ void ecryptfs_put_lower_file(struct inode *inode)
 	}
 }
 
-enum { ecryptfs_opt_sig, ecryptfs_opt_ecryptfs_sig,
-       ecryptfs_opt_cipher, ecryptfs_opt_ecryptfs_cipher,
-       ecryptfs_opt_ecryptfs_key_bytes,
-       ecryptfs_opt_passthrough, ecryptfs_opt_xattr_metadata,
-       ecryptfs_opt_encrypted_view, ecryptfs_opt_fnek_sig,
-       ecryptfs_opt_fn_cipher, ecryptfs_opt_fn_cipher_key_bytes,
-       ecryptfs_opt_unlink_sigs, ecryptfs_opt_mount_auth_tok_only,
-       ecryptfs_opt_check_dev_ruid,
-       ecryptfs_opt_err };
-
-static const match_table_t tokens = {
-	{ecryptfs_opt_sig, "sig=%s"},
-	{ecryptfs_opt_ecryptfs_sig, "ecryptfs_sig=%s"},
-	{ecryptfs_opt_cipher, "cipher=%s"},
-	{ecryptfs_opt_ecryptfs_cipher, "ecryptfs_cipher=%s"},
-	{ecryptfs_opt_ecryptfs_key_bytes, "ecryptfs_key_bytes=%u"},
-	{ecryptfs_opt_passthrough, "ecryptfs_passthrough"},
-	{ecryptfs_opt_xattr_metadata, "ecryptfs_xattr_metadata"},
-	{ecryptfs_opt_encrypted_view, "ecryptfs_encrypted_view"},
-	{ecryptfs_opt_fnek_sig, "ecryptfs_fnek_sig=%s"},
-	{ecryptfs_opt_fn_cipher, "ecryptfs_fn_cipher=%s"},
-	{ecryptfs_opt_fn_cipher_key_bytes, "ecryptfs_fn_key_bytes=%u"},
-	{ecryptfs_opt_unlink_sigs, "ecryptfs_unlink_sigs"},
-	{ecryptfs_opt_mount_auth_tok_only, "ecryptfs_mount_auth_tok_only"},
-	{ecryptfs_opt_check_dev_ruid, "ecryptfs_check_dev_ruid"},
-	{ecryptfs_opt_err, NULL}
+enum { Opt_sig, Opt_ecryptfs_sig,
+       Opt_cipher, Opt_ecryptfs_cipher,
+       Opt_ecryptfs_key_bytes,
+       Opt_passthrough, Opt_xattr_metadata,
+       Opt_encrypted_view, Opt_fnek_sig,
+       Opt_fn_cipher, Opt_fn_cipher_key_bytes,
+       Opt_unlink_sigs, Opt_mount_auth_tok_only,
+       Opt_check_dev_ruid };
+
+static const struct fs_parameter_spec ecryptfs_fs_param_spec[] = {
+	fsparam_string	("sig",			    Opt_sig),
+	fsparam_string	("ecryptfs_sig",	    Opt_ecryptfs_sig),
+	fsparam_string	("cipher",		    Opt_cipher),
+	fsparam_string	("ecryptfs_cipher",	    Opt_ecryptfs_cipher),
+	fsparam_u32	("ecryptfs_key_bytes",	    Opt_ecryptfs_key_bytes),
+	fsparam_flag	("ecryptfs_passthrough",    Opt_passthrough),
+	fsparam_flag	("ecryptfs_xattr_metadata", Opt_xattr_metadata),
+	fsparam_flag	("ecryptfs_encrypted_view", Opt_encrypted_view),
+	fsparam_string	("ecryptfs_fnek_sig",	    Opt_fnek_sig),
+	fsparam_string	("ecryptfs_fn_cipher",	    Opt_fn_cipher),
+	fsparam_u32	("ecryptfs_fn_key_bytes",   Opt_fn_cipher_key_bytes),
+	fsparam_flag	("ecryptfs_unlink_sigs",    Opt_unlink_sigs),
+	fsparam_flag	("ecryptfs_mount_auth_tok_only", Opt_mount_auth_tok_only),
+	fsparam_flag	("ecryptfs_check_dev_ruid", Opt_check_dev_ruid),
+	{}
 };
 
 static int ecryptfs_init_global_auth_toks(
@@ -219,19 +218,20 @@ static void ecryptfs_init_mount_crypt_stat(
 	mount_crypt_stat->flags |= ECRYPTFS_MOUNT_CRYPT_STAT_INITIALIZED;
 }
 
+struct ecryptfs_fs_context {
+	/* Mount option status trackers */
+	bool check_ruid;
+	bool sig_set;
+	bool cipher_name_set;
+	bool cipher_key_bytes_set;
+	bool fn_cipher_name_set;
+	bool fn_cipher_key_bytes_set;
+};
+
 /**
- * ecryptfs_parse_options
- * @sbi: The ecryptfs super block
- * @options: The options passed to the kernel
- * @check_ruid: set to 1 if device uid should be checked against the ruid
- *
- * Parse mount options:
- * debug=N 	   - ecryptfs_verbosity level for debug output
- * sig=XXX	   - description(signature) of the key to use
- *
- * Returns the dentry object of the lower-level (lower/interposed)
- * directory; We want to mount our stackable file system on top of
- * that lower directory.
+ * ecryptfs_parse_param
+ * @fc: The ecryptfs filesystem context
+ * @param: The mount parameter to parse
  *
  * The signature of the key to use must be the description of a key
  * already in the keyring. Mounting will fail if the key can not be
@@ -239,145 +239,118 @@ static void ecryptfs_init_mount_crypt_stat(
  *
  * Returns zero on success; non-zero on error
  */
-static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options)
+static int ecryptfs_parse_param(
+	struct fs_context *fc,
+	struct fs_parameter *param)
 {
-	char *p;
-	int rc = 0;
-	int cipher_key_bytes;
-	int fn_cipher_key_bytes;
+	int rc;
+	int opt;
+	struct fs_parse_result result;
+	struct ecryptfs_fs_context *ctx = fc->fs_private;
+	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
 		&sbi->mount_crypt_stat;
-	substring_t args[MAX_OPT_ARGS];
-	int token;
-	char *sig_src;
-	char *cipher_name_src;
-	char *fn_cipher_name_src;
-	char *fnek_src;
-	char *cipher_key_bytes_src;
-	char *fn_cipher_key_bytes_src;
-
-	if (!options) {
-		rc = -EINVAL;
-		goto out;
-	}
-	ecryptfs_init_mount_crypt_stat(mount_crypt_stat);
-	while ((p = strsep(&options, ",")) != NULL) {
-		if (!*p)
-			continue;
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case ecryptfs_opt_sig:
-		case ecryptfs_opt_ecryptfs_sig:
-			sig_src = args[0].from;
-			rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
-							  sig_src, 0);
-			if (rc) {
-				printk(KERN_ERR "Error attempting to register "
-				       "global sig; rc = [%d]\n", rc);
-				goto out;
-			}
-			mount_crypt_stat->sig_set = 1;
-			break;
-		case ecryptfs_opt_cipher:
-		case ecryptfs_opt_ecryptfs_cipher:
-			cipher_name_src = args[0].from;
-			strscpy(mount_crypt_stat->global_default_cipher_name,
-				cipher_name_src);
-			mount_crypt_stat->cipher_name_set = 1;
-			break;
-		case ecryptfs_opt_ecryptfs_key_bytes:
-			cipher_key_bytes_src = args[0].from;
-			cipher_key_bytes =
-				(int)simple_strtol(cipher_key_bytes_src,
-						   &cipher_key_bytes_src, 0);
-			mount_crypt_stat->global_default_cipher_key_size =
-				cipher_key_bytes;
-			mount_crypt_stat->cipher_key_bytes_set = 1;
-			break;
-		case ecryptfs_opt_passthrough:
-			mount_crypt_stat->flags |=
-				ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED;
-			break;
-		case ecryptfs_opt_xattr_metadata:
-			mount_crypt_stat->flags |=
-				ECRYPTFS_XATTR_METADATA_ENABLED;
-			break;
-		case ecryptfs_opt_encrypted_view:
-			mount_crypt_stat->flags |=
-				ECRYPTFS_XATTR_METADATA_ENABLED;
-			mount_crypt_stat->flags |=
-				ECRYPTFS_ENCRYPTED_VIEW_ENABLED;
-			break;
-		case ecryptfs_opt_fnek_sig:
-			fnek_src = args[0].from;
-			strscpy(mount_crypt_stat->global_default_fnek_sig,
-				fnek_src);
-			rc = ecryptfs_add_global_auth_tok(
-				mount_crypt_stat,
-				mount_crypt_stat->global_default_fnek_sig,
-				ECRYPTFS_AUTH_TOK_FNEK);
-			if (rc) {
-				printk(KERN_ERR "Error attempting to register "
-				       "global fnek sig [%s]; rc = [%d]\n",
-				       mount_crypt_stat->global_default_fnek_sig,
-				       rc);
-				goto out;
-			}
-			mount_crypt_stat->flags |=
-				(ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES
-				 | ECRYPTFS_GLOBAL_ENCFN_USE_MOUNT_FNEK);
-			break;
-		case ecryptfs_opt_fn_cipher:
-			fn_cipher_name_src = args[0].from;
-			strscpy(mount_crypt_stat->global_default_fn_cipher_name,
-				fn_cipher_name_src);
-			mount_crypt_stat->fn_cipher_name_set = 1;
-			break;
-		case ecryptfs_opt_fn_cipher_key_bytes:
-			fn_cipher_key_bytes_src = args[0].from;
-			fn_cipher_key_bytes =
-				(int)simple_strtol(fn_cipher_key_bytes_src,
-						   &fn_cipher_key_bytes_src, 0);
-			mount_crypt_stat->global_default_fn_cipher_key_bytes =
-				fn_cipher_key_bytes;
-			mount_crypt_stat->fn_cipher_key_bytes_set = 1;
-			break;
-		case ecryptfs_opt_unlink_sigs:
-			mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
-			break;
-		case ecryptfs_opt_mount_auth_tok_only:
-			mount_crypt_stat->flags |=
-				ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
-			break;
-		case ecryptfs_opt_check_dev_ruid:
-			mount_crypt_stat->check_ruid = 1;
-			break;
-		case ecryptfs_opt_err:
-		default:
-			printk(KERN_WARNING
-			       "%s: eCryptfs: unrecognized option [%s]\n",
-			       __func__, p);
+
+	opt = fs_parse(fc, ecryptfs_fs_param_spec, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
+	case Opt_sig:
+	case Opt_ecryptfs_sig:
+		rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
+						  param->string, 0);
+		if (rc) {
+			printk(KERN_ERR "Error attempting to register "
+			       "global sig; rc = [%d]\n", rc);
+			return rc;;
 		}
+		ctx->sig_set = 1;
+		break;
+	case Opt_cipher:
+	case Opt_ecryptfs_cipher:
+		strscpy(mount_crypt_stat->global_default_cipher_name,
+			param->string);
+		ctx->cipher_name_set = 1;
+		break;
+	case Opt_ecryptfs_key_bytes:
+		mount_crypt_stat->global_default_cipher_key_size =
+			result.uint_32;
+		ctx->cipher_key_bytes_set = 1;
+		break;
+	case Opt_passthrough:
+		mount_crypt_stat->flags |=
+			ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED;
+		break;
+	case Opt_xattr_metadata:
+		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
+		break;
+	case Opt_encrypted_view:
+		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
+		mount_crypt_stat->flags |= ECRYPTFS_ENCRYPTED_VIEW_ENABLED;
+		break;
+	case Opt_fnek_sig:
+		strscpy(mount_crypt_stat->global_default_fnek_sig,
+			param->string);
+		rc = ecryptfs_add_global_auth_tok(
+			mount_crypt_stat,
+			mount_crypt_stat->global_default_fnek_sig,
+			ECRYPTFS_AUTH_TOK_FNEK);
+		if (rc) {
+			printk(KERN_ERR "Error attempting to register "
+			       "global fnek sig [%s]; rc = [%d]\n",
+			       mount_crypt_stat->global_default_fnek_sig, rc);
+			return rc;
+		}
+		mount_crypt_stat->flags |=
+			(ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES
+			 | ECRYPTFS_GLOBAL_ENCFN_USE_MOUNT_FNEK);
+		break;
+	case Opt_fn_cipher:
+		strscpy(mount_crypt_stat->global_default_fn_cipher_name,
+			param->string);
+		ctx->fn_cipher_name_set = 1;
+		break;
+	case Opt_fn_cipher_key_bytes:
+		mount_crypt_stat->global_default_fn_cipher_key_bytes =
+			result.uint_32;
+		ctx->fn_cipher_key_bytes_set = 1;
+		break;
+	case Opt_unlink_sigs:
+		mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
+		break;
+	case Opt_mount_auth_tok_only:
+		mount_crypt_stat->flags |= ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
+		break;
+	case Opt_check_dev_ruid:
+		ctx->check_ruid = 1;
+		break;
+	default:
+		return -EINVAL;
 	}
 
-out:
-	return rc;
+	return 0;
 }
 
-static int ecryptfs_validate_options(
-		struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
+static int ecryptfs_validate_options(struct fs_context *fc)
 {
 	int rc = 0;
 	u8 cipher_code;
+	struct ecryptfs_fs_context *ctx = fc->fs_private;
+	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
+	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
+
+
+	mount_crypt_stat = &sbi->mount_crypt_stat;
 
-	if (!mount_crypt_stat->sig_set) {
+	if (!ctx->sig_set) {
 		rc = -EINVAL;
 		ecryptfs_printk(KERN_ERR, "You must supply at least one valid "
 				"auth tok signature as a mount "
 				"parameter; see the eCryptfs README\n");
 		goto out;
 	}
-	if (!mount_crypt_stat->cipher_name_set) {
+	if (!ctx->cipher_name_set) {
 		int cipher_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER);
 
 		BUG_ON(cipher_name_len > ECRYPTFS_MAX_CIPHER_NAME_SIZE);
@@ -385,13 +358,13 @@ static int ecryptfs_validate_options(
 		       ECRYPTFS_DEFAULT_CIPHER);
 	}
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
-	    && !mount_crypt_stat->fn_cipher_name_set)
+	    && !ctx->fn_cipher_name_set)
 		strcpy(mount_crypt_stat->global_default_fn_cipher_name,
 		       mount_crypt_stat->global_default_cipher_name);
-	if (!mount_crypt_stat->cipher_key_bytes_set)
+	if (!ctx->cipher_key_bytes_set)
 		mount_crypt_stat->global_default_cipher_key_size = 0;
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
-	    && !mount_crypt_stat->fn_cipher_key_bytes_set)
+	    && !ctx->fn_cipher_key_bytes_set)
 		mount_crypt_stat->global_default_fn_cipher_key_bytes =
 			mount_crypt_stat->global_default_cipher_key_size;
 
@@ -455,17 +428,14 @@ struct kmem_cache *ecryptfs_sb_info_cache;
 static struct file_system_type ecryptfs_fs_type;
 
 /*
- * ecryptfs_mount
- * @fs_type: The filesystem type that the superblock should belong to
- * @flags: The flags associated with the mount
- * @dev_name: The path to mount over
- * @raw_data: The options passed into the kernel
+ * ecryptfs_get_tree
+ * @fc: The filesystem context
  */
-static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags,
-			const char *dev_name, void *raw_data)
+static int ecryptfs_get_tree(struct fs_context *fc)
 {
 	struct super_block *s;
-	struct ecryptfs_sb_info *sbi;
+	struct ecryptfs_fs_context *ctx = fc->fs_private;
+	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
 	struct ecryptfs_dentry_info *root_info;
 	const char *err = "Getting sb failed";
@@ -473,31 +443,20 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	struct path path;
 	int rc;
 
-	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
-	if (!sbi) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
-	if (!dev_name) {
+	if (!fc->source) {
 		rc = -EINVAL;
 		err = "Device name cannot be null";
 		goto out;
 	}
 
-	rc = ecryptfs_parse_options(sbi, raw_data);
-	if (rc) {
-		err = "Error parsing options";
-		goto out;
-	}
 	mount_crypt_stat = &sbi->mount_crypt_stat;
-	rc = ecryptfs_validate_options(mount_crypt_stat);
+	rc = ecryptfs_validate_options(fc);
 	if (rc) {
 		err = "Error validationg options";
 		goto out;
 	}
 
-	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
+	s = sget_fc(fc, NULL, set_anon_super_fc);
 	if (IS_ERR(s)) {
 		rc = PTR_ERR(s);
 		goto out;
@@ -516,7 +475,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	s->s_d_op = &ecryptfs_dops;
 
 	err = "Reading sb failed";
-	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
+	rc = kern_path(fc->source, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
 	if (rc) {
 		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
 		goto out1;
@@ -535,7 +494,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 		goto out_free;
 	}
 
-	if (mount_crypt_stat->check_ruid &&
+	if (ctx->check_ruid &&
 	    !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
 		rc = -EPERM;
 		printk(KERN_ERR "Mount of device (uid: %d) not owned by "
@@ -551,7 +510,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	 * Set the POSIX ACL flag based on whether they're enabled in the lower
 	 * mount.
 	 */
-	s->s_flags = flags & ~SB_POSIXACL;
+	s->s_flags = fc->sb_flags & ~SB_POSIXACL;
 	s->s_flags |= path.dentry->d_sb->s_flags & SB_POSIXACL;
 
 	/**
@@ -594,19 +553,19 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	root_info->lower_path = path;
 
 	s->s_flags |= SB_ACTIVE;
-	return dget(s->s_root);
+	fc->root = dget(s->s_root);
+	return 0;
 
 out_free:
 	path_put(&path);
 out1:
 	deactivate_locked_super(s);
 out:
-	if (sbi) {
+	if (sbi)
 		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
-		kmem_cache_free(ecryptfs_sb_info_cache, sbi);
-	}
+
 	printk(KERN_ERR "%s; rc = [%d]\n", err, rc);
-	return ERR_PTR(rc);
+	return rc;
 }
 
 /**
@@ -625,10 +584,54 @@ static void ecryptfs_kill_block_super(struct super_block *sb)
 	kmem_cache_free(ecryptfs_sb_info_cache, sb_info);
 }
 
+static void ecryptfs_free_fc(struct fs_context *fc)
+{
+	struct ecryptfs_fs_context *ctx = fc->fs_private;
+	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
+
+	kfree(ctx);
+
+	if (sbi) {
+		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
+		kmem_cache_free(ecryptfs_sb_info_cache, sbi);
+	}
+}
+
+static const struct fs_context_operations ecryptfs_context_ops = {
+	.free		= ecryptfs_free_fc,
+	.parse_param	= ecryptfs_parse_param,
+	.get_tree	= ecryptfs_get_tree,
+	.reconfigure	= NULL,
+};
+
+static int ecryptfs_init_fs_context(struct fs_context *fc)
+{
+	struct ecryptfs_fs_context *ctx;
+	struct ecryptfs_sb_info *sbi = NULL;
+
+	ctx = kzalloc(sizeof(struct ecryptfs_fs_context), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
+	if (!sbi) {
+		kfree(ctx);
+		ctx = NULL;
+		return -ENOMEM;
+	}
+
+	ecryptfs_init_mount_crypt_stat(&sbi->mount_crypt_stat);
+
+	fc->fs_private = ctx;
+	fc->s_fs_info = sbi;
+	fc->ops = &ecryptfs_context_ops;
+	return 0;
+}
+
 static struct file_system_type ecryptfs_fs_type = {
 	.owner = THIS_MODULE,
 	.name = "ecryptfs",
-	.mount = ecryptfs_mount,
+	.init_fs_context = ecryptfs_init_fs_context,
+	.parameters = ecryptfs_fs_param_spec,
 	.kill_sb = ecryptfs_kill_block_super,
 	.fs_flags = 0
 };
-- 
2.46.0

Re: [PATCH 0/2] ecryptfs: convert to the new mount API

[Eric Sandeen]

On 10/7/24 10:27 AM, Eric Sandeen wrote:
> This is lightly tested with the kernel tests present in ecryptfs-utils,
> but it could certainly use a bit more testing and review, particularly
> with invalid mount option sets.
> 
> This one is a little unique compared to other filesystems in that I
> allocate both an fs context and the *sbi in .init_fs_context; the *sbi
> is long-lived, and the context is only present during the initial mount.
> 
> Allocating sbi with the filesystem context means we can set options
> into it directly, rather than needing to do it after parsing. And it's
> particularly simple to do it this way given that there is no remount.
> 
> I could squash these two patches into one if you prefer, but
> I thought maybe breaking out the first change made review a little
> easier.
> 
> Thanks,
> -Eric
> 

Ping on this, I guess.

In theory ecryptfs is slated to be deprecated and removed, per
https://lore.kernel.org/ecryptfs/ZCuSLNnFQEdOHW0c@sequoia/ but I'm not
sure that patch ever got sent.

I think this conversion is simple enough, and given that ecrypytfs is still
in the tree, it'll stand in the way of removing the old infrastructure
whenever the last filesystem finally gets converted, but to be fair we have
a little ways to go there.

If it's really going to be removed soon (ala reiserfs) then I suppose these
patches can be ignored.

Thanks,
-Eric

Re: [PATCH 1/2] ecryptfs: Factor out mount option validation

[Tyler Hicks]

On 2024-10-07 10:27:42, Eric Sandeen wrote:
> Under the new mount API, mount options are parsed one at a time.
> Any validation that examines multiple options must be done after parsing
> is complete, so factor out a ecryptfs_validate_options() which can be
> called separately.
> 
> To facilitate this, temporarily move the local variables that tracked
> whether various options have been set in the parsing function, into the
> ecryptfs_mount_crypt_stat structure so that they can be examined later.
> 
> These will be moved to a more ephemeral struct in the mount api conversion
> patch to follow.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Thanks, Eric!

I was going to push back on storing the *_set bools in the
ecryptfs_mount_crypt_stat struct since those bools only meant to be
short lived and that struct lives for the lifetime of the mount.
However, I see you clean that up in the following patch.

Acked-by: Tyler Hicks <code@tyhicks.com>

Tyler

> ---
>  fs/ecryptfs/ecryptfs_kernel.h |  7 +++++
>  fs/ecryptfs/main.c            | 55 ++++++++++++++++++++---------------
>  2 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index c586c5db18b5..d359ec085a70 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -343,6 +343,13 @@ struct ecryptfs_mount_crypt_stat {
>  	unsigned char global_default_fn_cipher_name[
>  		ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
>  	char global_default_fnek_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
> +	/* Mount option status trackers */
> +	bool check_ruid;
> +	bool sig_set;
> +	bool cipher_name_set;
> +	bool cipher_key_bytes_set;
> +	bool fn_cipher_name_set;
> +	bool fn_cipher_key_bytes_set;
>  };
>  
>  /* superblock private data. */
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index 577c56302314..d03f1c6ccc1c 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -239,18 +239,12 @@ static void ecryptfs_init_mount_crypt_stat(
>   *
>   * Returns zero on success; non-zero on error
>   */
> -static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
> -				  uid_t *check_ruid)
> +static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options)
>  {
>  	char *p;
>  	int rc = 0;
> -	int sig_set = 0;
> -	int cipher_name_set = 0;
> -	int fn_cipher_name_set = 0;
>  	int cipher_key_bytes;
> -	int cipher_key_bytes_set = 0;
>  	int fn_cipher_key_bytes;
> -	int fn_cipher_key_bytes_set = 0;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
>  		&sbi->mount_crypt_stat;
>  	substring_t args[MAX_OPT_ARGS];
> @@ -261,9 +255,6 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  	char *fnek_src;
>  	char *cipher_key_bytes_src;
>  	char *fn_cipher_key_bytes_src;
> -	u8 cipher_code;
> -
> -	*check_ruid = 0;
>  
>  	if (!options) {
>  		rc = -EINVAL;
> @@ -285,14 +276,14 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  				       "global sig; rc = [%d]\n", rc);
>  				goto out;
>  			}
> -			sig_set = 1;
> +			mount_crypt_stat->sig_set = 1;
>  			break;
>  		case ecryptfs_opt_cipher:
>  		case ecryptfs_opt_ecryptfs_cipher:
>  			cipher_name_src = args[0].from;
>  			strscpy(mount_crypt_stat->global_default_cipher_name,
>  				cipher_name_src);
> -			cipher_name_set = 1;
> +			mount_crypt_stat->cipher_name_set = 1;
>  			break;
>  		case ecryptfs_opt_ecryptfs_key_bytes:
>  			cipher_key_bytes_src = args[0].from;
> @@ -301,7 +292,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  						   &cipher_key_bytes_src, 0);
>  			mount_crypt_stat->global_default_cipher_key_size =
>  				cipher_key_bytes;
> -			cipher_key_bytes_set = 1;
> +			mount_crypt_stat->cipher_key_bytes_set = 1;
>  			break;
>  		case ecryptfs_opt_passthrough:
>  			mount_crypt_stat->flags |=
> @@ -340,7 +331,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  			fn_cipher_name_src = args[0].from;
>  			strscpy(mount_crypt_stat->global_default_fn_cipher_name,
>  				fn_cipher_name_src);
> -			fn_cipher_name_set = 1;
> +			mount_crypt_stat->fn_cipher_name_set = 1;
>  			break;
>  		case ecryptfs_opt_fn_cipher_key_bytes:
>  			fn_cipher_key_bytes_src = args[0].from;
> @@ -349,7 +340,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  						   &fn_cipher_key_bytes_src, 0);
>  			mount_crypt_stat->global_default_fn_cipher_key_bytes =
>  				fn_cipher_key_bytes;
> -			fn_cipher_key_bytes_set = 1;
> +			mount_crypt_stat->fn_cipher_key_bytes_set = 1;
>  			break;
>  		case ecryptfs_opt_unlink_sigs:
>  			mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
> @@ -359,7 +350,7 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  				ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
>  			break;
>  		case ecryptfs_opt_check_dev_ruid:
> -			*check_ruid = 1;
> +			mount_crypt_stat->check_ruid = 1;
>  			break;
>  		case ecryptfs_opt_err:
>  		default:
> @@ -368,14 +359,25 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  			       __func__, p);
>  		}
>  	}
> -	if (!sig_set) {
> +
> +out:
> +	return rc;
> +}
> +
> +static int ecryptfs_validate_options(
> +		struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> +{
> +	int rc = 0;
> +	u8 cipher_code;
> +
> +	if (!mount_crypt_stat->sig_set) {
>  		rc = -EINVAL;
>  		ecryptfs_printk(KERN_ERR, "You must supply at least one valid "
>  				"auth tok signature as a mount "
>  				"parameter; see the eCryptfs README\n");
>  		goto out;
>  	}
> -	if (!cipher_name_set) {
> +	if (!mount_crypt_stat->cipher_name_set) {
>  		int cipher_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER);
>  
>  		BUG_ON(cipher_name_len > ECRYPTFS_MAX_CIPHER_NAME_SIZE);
> @@ -383,13 +385,13 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  		       ECRYPTFS_DEFAULT_CIPHER);
>  	}
>  	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
> -	    && !fn_cipher_name_set)
> +	    && !mount_crypt_stat->fn_cipher_name_set)
>  		strcpy(mount_crypt_stat->global_default_fn_cipher_name,
>  		       mount_crypt_stat->global_default_cipher_name);
> -	if (!cipher_key_bytes_set)
> +	if (!mount_crypt_stat->cipher_key_bytes_set)
>  		mount_crypt_stat->global_default_cipher_key_size = 0;
>  	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
> -	    && !fn_cipher_key_bytes_set)
> +	    && !mount_crypt_stat->fn_cipher_key_bytes_set)
>  		mount_crypt_stat->global_default_fn_cipher_key_bytes =
>  			mount_crypt_stat->global_default_cipher_key_size;
>  
> @@ -469,7 +471,6 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	const char *err = "Getting sb failed";
>  	struct inode *inode;
>  	struct path path;
> -	uid_t check_ruid;
>  	int rc;
>  
>  	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
> @@ -484,12 +485,17 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  		goto out;
>  	}
>  
> -	rc = ecryptfs_parse_options(sbi, raw_data, &check_ruid);
> +	rc = ecryptfs_parse_options(sbi, raw_data);
>  	if (rc) {
>  		err = "Error parsing options";
>  		goto out;
>  	}
>  	mount_crypt_stat = &sbi->mount_crypt_stat;
> +	rc = ecryptfs_validate_options(mount_crypt_stat);
> +	if (rc) {
> +		err = "Error validationg options";
> +		goto out;
> +	}
>  
>  	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
>  	if (IS_ERR(s)) {
> @@ -529,7 +535,8 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  		goto out_free;
>  	}
>  
> -	if (check_ruid && !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
> +	if (mount_crypt_stat->check_ruid &&
> +	    !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
>  		rc = -EPERM;
>  		printk(KERN_ERR "Mount of device (uid: %d) not owned by "
>  		       "requested user (uid: %d)\n",
> -- 
> 2.46.0
> 

Re: [PATCH 2/2] ecryptfs: Convert ecryptfs to use the new mount API

[Tyler Hicks]

On 2024-10-07 10:27:43, Eric Sandeen wrote:
> Convert ecryptfs to the new mount API.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/ecryptfs/ecryptfs_kernel.h |   7 -
>  fs/ecryptfs/main.c            | 393 +++++++++++++++++-----------------
>  2 files changed, 198 insertions(+), 202 deletions(-)
> 
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index d359ec085a70..c586c5db18b5 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -343,13 +343,6 @@ struct ecryptfs_mount_crypt_stat {
>  	unsigned char global_default_fn_cipher_name[
>  		ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
>  	char global_default_fnek_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
> -	/* Mount option status trackers */
> -	bool check_ruid;
> -	bool sig_set;
> -	bool cipher_name_set;
> -	bool cipher_key_bytes_set;
> -	bool fn_cipher_name_set;
> -	bool fn_cipher_key_bytes_set;
>  };
>  
>  /* superblock private data. */
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index d03f1c6ccc1c..a7829983304e 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -15,10 +15,10 @@
>  #include <linux/module.h>
>  #include <linux/namei.h>
>  #include <linux/skbuff.h>
> -#include <linux/mount.h>
>  #include <linux/pagemap.h>
>  #include <linux/key.h>
> -#include <linux/parser.h>
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
>  #include <linux/fs_stack.h>
>  #include <linux/slab.h>
>  #include <linux/magic.h>
> @@ -153,32 +153,31 @@ void ecryptfs_put_lower_file(struct inode *inode)
>  	}
>  }
>  
> -enum { ecryptfs_opt_sig, ecryptfs_opt_ecryptfs_sig,
> -       ecryptfs_opt_cipher, ecryptfs_opt_ecryptfs_cipher,
> -       ecryptfs_opt_ecryptfs_key_bytes,
> -       ecryptfs_opt_passthrough, ecryptfs_opt_xattr_metadata,
> -       ecryptfs_opt_encrypted_view, ecryptfs_opt_fnek_sig,
> -       ecryptfs_opt_fn_cipher, ecryptfs_opt_fn_cipher_key_bytes,
> -       ecryptfs_opt_unlink_sigs, ecryptfs_opt_mount_auth_tok_only,
> -       ecryptfs_opt_check_dev_ruid,
> -       ecryptfs_opt_err };
> -
> -static const match_table_t tokens = {
> -	{ecryptfs_opt_sig, "sig=%s"},
> -	{ecryptfs_opt_ecryptfs_sig, "ecryptfs_sig=%s"},
> -	{ecryptfs_opt_cipher, "cipher=%s"},
> -	{ecryptfs_opt_ecryptfs_cipher, "ecryptfs_cipher=%s"},
> -	{ecryptfs_opt_ecryptfs_key_bytes, "ecryptfs_key_bytes=%u"},
> -	{ecryptfs_opt_passthrough, "ecryptfs_passthrough"},
> -	{ecryptfs_opt_xattr_metadata, "ecryptfs_xattr_metadata"},
> -	{ecryptfs_opt_encrypted_view, "ecryptfs_encrypted_view"},
> -	{ecryptfs_opt_fnek_sig, "ecryptfs_fnek_sig=%s"},
> -	{ecryptfs_opt_fn_cipher, "ecryptfs_fn_cipher=%s"},
> -	{ecryptfs_opt_fn_cipher_key_bytes, "ecryptfs_fn_key_bytes=%u"},
> -	{ecryptfs_opt_unlink_sigs, "ecryptfs_unlink_sigs"},
> -	{ecryptfs_opt_mount_auth_tok_only, "ecryptfs_mount_auth_tok_only"},
> -	{ecryptfs_opt_check_dev_ruid, "ecryptfs_check_dev_ruid"},
> -	{ecryptfs_opt_err, NULL}
> +enum { Opt_sig, Opt_ecryptfs_sig,
> +       Opt_cipher, Opt_ecryptfs_cipher,
> +       Opt_ecryptfs_key_bytes,
> +       Opt_passthrough, Opt_xattr_metadata,
> +       Opt_encrypted_view, Opt_fnek_sig,
> +       Opt_fn_cipher, Opt_fn_cipher_key_bytes,
> +       Opt_unlink_sigs, Opt_mount_auth_tok_only,
> +       Opt_check_dev_ruid };

checkpatch complains about these lines starting with spaces rather than
a tab. I think that's a valid nit. Can we switch to tabs?

> +
> +static const struct fs_parameter_spec ecryptfs_fs_param_spec[] = {
> +	fsparam_string	("sig",			    Opt_sig),
> +	fsparam_string	("ecryptfs_sig",	    Opt_ecryptfs_sig),
> +	fsparam_string	("cipher",		    Opt_cipher),
> +	fsparam_string	("ecryptfs_cipher",	    Opt_ecryptfs_cipher),
> +	fsparam_u32	("ecryptfs_key_bytes",	    Opt_ecryptfs_key_bytes),
> +	fsparam_flag	("ecryptfs_passthrough",    Opt_passthrough),
> +	fsparam_flag	("ecryptfs_xattr_metadata", Opt_xattr_metadata),
> +	fsparam_flag	("ecryptfs_encrypted_view", Opt_encrypted_view),
> +	fsparam_string	("ecryptfs_fnek_sig",	    Opt_fnek_sig),
> +	fsparam_string	("ecryptfs_fn_cipher",	    Opt_fn_cipher),
> +	fsparam_u32	("ecryptfs_fn_key_bytes",   Opt_fn_cipher_key_bytes),
> +	fsparam_flag	("ecryptfs_unlink_sigs",    Opt_unlink_sigs),
> +	fsparam_flag	("ecryptfs_mount_auth_tok_only", Opt_mount_auth_tok_only),
> +	fsparam_flag	("ecryptfs_check_dev_ruid", Opt_check_dev_ruid),
> +	{}
>  };
>  
>  static int ecryptfs_init_global_auth_toks(
> @@ -219,19 +218,20 @@ static void ecryptfs_init_mount_crypt_stat(
>  	mount_crypt_stat->flags |= ECRYPTFS_MOUNT_CRYPT_STAT_INITIALIZED;
>  }
>  
> +struct ecryptfs_fs_context {
> +	/* Mount option status trackers */
> +	bool check_ruid;
> +	bool sig_set;
> +	bool cipher_name_set;
> +	bool cipher_key_bytes_set;
> +	bool fn_cipher_name_set;
> +	bool fn_cipher_key_bytes_set;
> +};
> +
>  /**
> - * ecryptfs_parse_options
> - * @sbi: The ecryptfs super block
> - * @options: The options passed to the kernel
> - * @check_ruid: set to 1 if device uid should be checked against the ruid
> - *
> - * Parse mount options:
> - * debug=N 	   - ecryptfs_verbosity level for debug output
> - * sig=XXX	   - description(signature) of the key to use
> - *
> - * Returns the dentry object of the lower-level (lower/interposed)
> - * directory; We want to mount our stackable file system on top of
> - * that lower directory.
> + * ecryptfs_parse_param
> + * @fc: The ecryptfs filesystem context
> + * @param: The mount parameter to parse
>   *
>   * The signature of the key to use must be the description of a key
>   * already in the keyring. Mounting will fail if the key can not be
> @@ -239,145 +239,118 @@ static void ecryptfs_init_mount_crypt_stat(
>   *
>   * Returns zero on success; non-zero on error
>   */
> -static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options)
> +static int ecryptfs_parse_param(
> +	struct fs_context *fc,
> +	struct fs_parameter *param)
>  {
> -	char *p;
> -	int rc = 0;
> -	int cipher_key_bytes;
> -	int fn_cipher_key_bytes;
> +	int rc;
> +	int opt;
> +	struct fs_parse_result result;
> +	struct ecryptfs_fs_context *ctx = fc->fs_private;
> +	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
>  		&sbi->mount_crypt_stat;
> -	substring_t args[MAX_OPT_ARGS];
> -	int token;
> -	char *sig_src;
> -	char *cipher_name_src;
> -	char *fn_cipher_name_src;
> -	char *fnek_src;
> -	char *cipher_key_bytes_src;
> -	char *fn_cipher_key_bytes_src;
> -
> -	if (!options) {
> -		rc = -EINVAL;
> -		goto out;
> -	}
> -	ecryptfs_init_mount_crypt_stat(mount_crypt_stat);
> -	while ((p = strsep(&options, ",")) != NULL) {
> -		if (!*p)
> -			continue;
> -		token = match_token(p, tokens, args);
> -		switch (token) {
> -		case ecryptfs_opt_sig:
> -		case ecryptfs_opt_ecryptfs_sig:
> -			sig_src = args[0].from;
> -			rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
> -							  sig_src, 0);
> -			if (rc) {
> -				printk(KERN_ERR "Error attempting to register "
> -				       "global sig; rc = [%d]\n", rc);
> -				goto out;
> -			}
> -			mount_crypt_stat->sig_set = 1;
> -			break;
> -		case ecryptfs_opt_cipher:
> -		case ecryptfs_opt_ecryptfs_cipher:
> -			cipher_name_src = args[0].from;
> -			strscpy(mount_crypt_stat->global_default_cipher_name,
> -				cipher_name_src);
> -			mount_crypt_stat->cipher_name_set = 1;
> -			break;
> -		case ecryptfs_opt_ecryptfs_key_bytes:
> -			cipher_key_bytes_src = args[0].from;
> -			cipher_key_bytes =
> -				(int)simple_strtol(cipher_key_bytes_src,
> -						   &cipher_key_bytes_src, 0);
> -			mount_crypt_stat->global_default_cipher_key_size =
> -				cipher_key_bytes;
> -			mount_crypt_stat->cipher_key_bytes_set = 1;
> -			break;
> -		case ecryptfs_opt_passthrough:
> -			mount_crypt_stat->flags |=
> -				ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED;
> -			break;
> -		case ecryptfs_opt_xattr_metadata:
> -			mount_crypt_stat->flags |=
> -				ECRYPTFS_XATTR_METADATA_ENABLED;
> -			break;
> -		case ecryptfs_opt_encrypted_view:
> -			mount_crypt_stat->flags |=
> -				ECRYPTFS_XATTR_METADATA_ENABLED;
> -			mount_crypt_stat->flags |=
> -				ECRYPTFS_ENCRYPTED_VIEW_ENABLED;
> -			break;
> -		case ecryptfs_opt_fnek_sig:
> -			fnek_src = args[0].from;
> -			strscpy(mount_crypt_stat->global_default_fnek_sig,
> -				fnek_src);
> -			rc = ecryptfs_add_global_auth_tok(
> -				mount_crypt_stat,
> -				mount_crypt_stat->global_default_fnek_sig,
> -				ECRYPTFS_AUTH_TOK_FNEK);
> -			if (rc) {
> -				printk(KERN_ERR "Error attempting to register "
> -				       "global fnek sig [%s]; rc = [%d]\n",
> -				       mount_crypt_stat->global_default_fnek_sig,
> -				       rc);
> -				goto out;
> -			}
> -			mount_crypt_stat->flags |=
> -				(ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES
> -				 | ECRYPTFS_GLOBAL_ENCFN_USE_MOUNT_FNEK);
> -			break;
> -		case ecryptfs_opt_fn_cipher:
> -			fn_cipher_name_src = args[0].from;
> -			strscpy(mount_crypt_stat->global_default_fn_cipher_name,
> -				fn_cipher_name_src);
> -			mount_crypt_stat->fn_cipher_name_set = 1;
> -			break;
> -		case ecryptfs_opt_fn_cipher_key_bytes:
> -			fn_cipher_key_bytes_src = args[0].from;
> -			fn_cipher_key_bytes =
> -				(int)simple_strtol(fn_cipher_key_bytes_src,
> -						   &fn_cipher_key_bytes_src, 0);
> -			mount_crypt_stat->global_default_fn_cipher_key_bytes =
> -				fn_cipher_key_bytes;
> -			mount_crypt_stat->fn_cipher_key_bytes_set = 1;
> -			break;
> -		case ecryptfs_opt_unlink_sigs:
> -			mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
> -			break;
> -		case ecryptfs_opt_mount_auth_tok_only:
> -			mount_crypt_stat->flags |=
> -				ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
> -			break;
> -		case ecryptfs_opt_check_dev_ruid:
> -			mount_crypt_stat->check_ruid = 1;
> -			break;
> -		case ecryptfs_opt_err:
> -		default:
> -			printk(KERN_WARNING
> -			       "%s: eCryptfs: unrecognized option [%s]\n",
> -			       __func__, p);

I think we lost this error message, which can be helpful for users when
debugging mount(2) failures.

See below in your new code where we handle the default case for what I
think we should include.

> +
> +	opt = fs_parse(fc, ecryptfs_fs_param_spec, param, &result);
> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_sig:
> +	case Opt_ecryptfs_sig:
> +		rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
> +						  param->string, 0);
> +		if (rc) {
> +			printk(KERN_ERR "Error attempting to register "
> +			       "global sig; rc = [%d]\n", rc);

Are we expected to be using errorf() and friends here rather than
printk()?

> +			return rc;;

There's an extra semicolon here.

>  		}
> +		ctx->sig_set = 1;
> +		break;
> +	case Opt_cipher:
> +	case Opt_ecryptfs_cipher:
> +		strscpy(mount_crypt_stat->global_default_cipher_name,
> +			param->string);
> +		ctx->cipher_name_set = 1;
> +		break;
> +	case Opt_ecryptfs_key_bytes:
> +		mount_crypt_stat->global_default_cipher_key_size =
> +			result.uint_32;
> +		ctx->cipher_key_bytes_set = 1;
> +		break;
> +	case Opt_passthrough:
> +		mount_crypt_stat->flags |=
> +			ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED;
> +		break;
> +	case Opt_xattr_metadata:
> +		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
> +		break;
> +	case Opt_encrypted_view:
> +		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
> +		mount_crypt_stat->flags |= ECRYPTFS_ENCRYPTED_VIEW_ENABLED;
> +		break;
> +	case Opt_fnek_sig:
> +		strscpy(mount_crypt_stat->global_default_fnek_sig,
> +			param->string);
> +		rc = ecryptfs_add_global_auth_tok(
> +			mount_crypt_stat,
> +			mount_crypt_stat->global_default_fnek_sig,
> +			ECRYPTFS_AUTH_TOK_FNEK);
> +		if (rc) {
> +			printk(KERN_ERR "Error attempting to register "
> +			       "global fnek sig [%s]; rc = [%d]\n",
> +			       mount_crypt_stat->global_default_fnek_sig, rc);

Same errorf() question here.

> +			return rc;
> +		}
> +		mount_crypt_stat->flags |=
> +			(ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES
> +			 | ECRYPTFS_GLOBAL_ENCFN_USE_MOUNT_FNEK);
> +		break;
> +	case Opt_fn_cipher:
> +		strscpy(mount_crypt_stat->global_default_fn_cipher_name,
> +			param->string);
> +		ctx->fn_cipher_name_set = 1;
> +		break;
> +	case Opt_fn_cipher_key_bytes:
> +		mount_crypt_stat->global_default_fn_cipher_key_bytes =
> +			result.uint_32;
> +		ctx->fn_cipher_key_bytes_set = 1;
> +		break;
> +	case Opt_unlink_sigs:
> +		mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
> +		break;
> +	case Opt_mount_auth_tok_only:
> +		mount_crypt_stat->flags |= ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
> +		break;
> +	case Opt_check_dev_ruid:
> +		ctx->check_ruid = 1;
> +		break;
> +	default:

To retain the unrecognized option warning, I think we need this:


                printk(KERN_WARNING "%s: eCryptfs: unrecognized option [%s]\n",
                       __func__, param->key);

Tyler

> +		return -EINVAL;
>  	}
>  
> -out:
> -	return rc;
> +	return 0;
>  }
>  
> -static int ecryptfs_validate_options(
> -		struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> +static int ecryptfs_validate_options(struct fs_context *fc)
>  {
>  	int rc = 0;
>  	u8 cipher_code;
> +	struct ecryptfs_fs_context *ctx = fc->fs_private;
> +	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
> +	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
> +
> +
> +	mount_crypt_stat = &sbi->mount_crypt_stat;
>  
> -	if (!mount_crypt_stat->sig_set) {
> +	if (!ctx->sig_set) {
>  		rc = -EINVAL;
>  		ecryptfs_printk(KERN_ERR, "You must supply at least one valid "
>  				"auth tok signature as a mount "
>  				"parameter; see the eCryptfs README\n");
>  		goto out;
>  	}
> -	if (!mount_crypt_stat->cipher_name_set) {
> +	if (!ctx->cipher_name_set) {
>  		int cipher_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER);
>  
>  		BUG_ON(cipher_name_len > ECRYPTFS_MAX_CIPHER_NAME_SIZE);
> @@ -385,13 +358,13 @@ static int ecryptfs_validate_options(
>  		       ECRYPTFS_DEFAULT_CIPHER);
>  	}
>  	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
> -	    && !mount_crypt_stat->fn_cipher_name_set)
> +	    && !ctx->fn_cipher_name_set)
>  		strcpy(mount_crypt_stat->global_default_fn_cipher_name,
>  		       mount_crypt_stat->global_default_cipher_name);
> -	if (!mount_crypt_stat->cipher_key_bytes_set)
> +	if (!ctx->cipher_key_bytes_set)
>  		mount_crypt_stat->global_default_cipher_key_size = 0;
>  	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
> -	    && !mount_crypt_stat->fn_cipher_key_bytes_set)
> +	    && !ctx->fn_cipher_key_bytes_set)
>  		mount_crypt_stat->global_default_fn_cipher_key_bytes =
>  			mount_crypt_stat->global_default_cipher_key_size;
>  
> @@ -455,17 +428,14 @@ struct kmem_cache *ecryptfs_sb_info_cache;
>  static struct file_system_type ecryptfs_fs_type;
>  
>  /*
> - * ecryptfs_mount
> - * @fs_type: The filesystem type that the superblock should belong to
> - * @flags: The flags associated with the mount
> - * @dev_name: The path to mount over
> - * @raw_data: The options passed into the kernel
> + * ecryptfs_get_tree
> + * @fc: The filesystem context
>   */
> -static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags,
> -			const char *dev_name, void *raw_data)
> +static int ecryptfs_get_tree(struct fs_context *fc)
>  {
>  	struct super_block *s;
> -	struct ecryptfs_sb_info *sbi;
> +	struct ecryptfs_fs_context *ctx = fc->fs_private;
> +	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
>  	struct ecryptfs_dentry_info *root_info;
>  	const char *err = "Getting sb failed";
> @@ -473,31 +443,20 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	struct path path;
>  	int rc;
>  
> -	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
> -	if (!sbi) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> -
> -	if (!dev_name) {
> +	if (!fc->source) {
>  		rc = -EINVAL;
>  		err = "Device name cannot be null";
>  		goto out;
>  	}
>  
> -	rc = ecryptfs_parse_options(sbi, raw_data);
> -	if (rc) {
> -		err = "Error parsing options";
> -		goto out;
> -	}
>  	mount_crypt_stat = &sbi->mount_crypt_stat;
> -	rc = ecryptfs_validate_options(mount_crypt_stat);
> +	rc = ecryptfs_validate_options(fc);
>  	if (rc) {
>  		err = "Error validationg options";
>  		goto out;
>  	}
>  
> -	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
> +	s = sget_fc(fc, NULL, set_anon_super_fc);
>  	if (IS_ERR(s)) {
>  		rc = PTR_ERR(s);
>  		goto out;
> @@ -516,7 +475,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	s->s_d_op = &ecryptfs_dops;
>  
>  	err = "Reading sb failed";
> -	rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
> +	rc = kern_path(fc->source, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
>  	if (rc) {
>  		ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
>  		goto out1;
> @@ -535,7 +494,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  		goto out_free;
>  	}
>  
> -	if (mount_crypt_stat->check_ruid &&
> +	if (ctx->check_ruid &&
>  	    !uid_eq(d_inode(path.dentry)->i_uid, current_uid())) {
>  		rc = -EPERM;
>  		printk(KERN_ERR "Mount of device (uid: %d) not owned by "
> @@ -551,7 +510,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	 * Set the POSIX ACL flag based on whether they're enabled in the lower
>  	 * mount.
>  	 */
> -	s->s_flags = flags & ~SB_POSIXACL;
> +	s->s_flags = fc->sb_flags & ~SB_POSIXACL;
>  	s->s_flags |= path.dentry->d_sb->s_flags & SB_POSIXACL;
>  
>  	/**
> @@ -594,19 +553,19 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	root_info->lower_path = path;
>  
>  	s->s_flags |= SB_ACTIVE;
> -	return dget(s->s_root);
> +	fc->root = dget(s->s_root);
> +	return 0;
>  
>  out_free:
>  	path_put(&path);
>  out1:
>  	deactivate_locked_super(s);
>  out:
> -	if (sbi) {
> +	if (sbi)
>  		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
> -		kmem_cache_free(ecryptfs_sb_info_cache, sbi);
> -	}
> +
>  	printk(KERN_ERR "%s; rc = [%d]\n", err, rc);
> -	return ERR_PTR(rc);
> +	return rc;
>  }
>  
>  /**
> @@ -625,10 +584,54 @@ static void ecryptfs_kill_block_super(struct super_block *sb)
>  	kmem_cache_free(ecryptfs_sb_info_cache, sb_info);
>  }
>  
> +static void ecryptfs_free_fc(struct fs_context *fc)
> +{
> +	struct ecryptfs_fs_context *ctx = fc->fs_private;
> +	struct ecryptfs_sb_info *sbi = fc->s_fs_info;
> +
> +	kfree(ctx);
> +
> +	if (sbi) {
> +		ecryptfs_destroy_mount_crypt_stat(&sbi->mount_crypt_stat);
> +		kmem_cache_free(ecryptfs_sb_info_cache, sbi);
> +	}
> +}
> +
> +static const struct fs_context_operations ecryptfs_context_ops = {
> +	.free		= ecryptfs_free_fc,
> +	.parse_param	= ecryptfs_parse_param,
> +	.get_tree	= ecryptfs_get_tree,
> +	.reconfigure	= NULL,
> +};
> +
> +static int ecryptfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct ecryptfs_fs_context *ctx;
> +	struct ecryptfs_sb_info *sbi = NULL;
> +
> +	ctx = kzalloc(sizeof(struct ecryptfs_fs_context), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
> +	if (!sbi) {
> +		kfree(ctx);
> +		ctx = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	ecryptfs_init_mount_crypt_stat(&sbi->mount_crypt_stat);
> +
> +	fc->fs_private = ctx;
> +	fc->s_fs_info = sbi;
> +	fc->ops = &ecryptfs_context_ops;
> +	return 0;
> +}
> +
>  static struct file_system_type ecryptfs_fs_type = {
>  	.owner = THIS_MODULE,
>  	.name = "ecryptfs",
> -	.mount = ecryptfs_mount,
> +	.init_fs_context = ecryptfs_init_fs_context,
> +	.parameters = ecryptfs_fs_param_spec,
>  	.kill_sb = ecryptfs_kill_block_super,
>  	.fs_flags = 0
>  };
> -- 
> 2.46.0
> 

Re: [PATCH 2/2] ecryptfs: Convert ecryptfs to use the new mount API

[Eric Sandeen]

On 10/21/24 1:09 AM, Tyler Hicks wrote:
> On 2024-10-07 10:27:43, Eric Sandeen wrote:
>> Convert ecryptfs to the new mount API.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/ecryptfs/ecryptfs_kernel.h |   7 -
>>  fs/ecryptfs/main.c            | 393 +++++++++++++++++-----------------
>>  2 files changed, 198 insertions(+), 202 deletions(-)
>>

...

>> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
>> index d03f1c6ccc1c..a7829983304e 100644
>> --- a/fs/ecryptfs/main.c
>> +++ b/fs/ecryptfs/main.c

...

>> +enum { Opt_sig, Opt_ecryptfs_sig,
>> +       Opt_cipher, Opt_ecryptfs_cipher,
>> +       Opt_ecryptfs_key_bytes,
>> +       Opt_passthrough, Opt_xattr_metadata,
>> +       Opt_encrypted_view, Opt_fnek_sig,
>> +       Opt_fn_cipher, Opt_fn_cipher_key_bytes,
>> +       Opt_unlink_sigs, Opt_mount_auth_tok_only,
>> +       Opt_check_dev_ruid };
> 
> checkpatch complains about these lines starting with spaces rather than
> a tab. I think that's a valid nit. Can we switch to tabs?

Oh, sure. They have spaces upstream and I didn't catch that when I tweaked
the enum.
 
>> -		case ecryptfs_opt_err:
>> -		default:
>> -			printk(KERN_WARNING
>> -			       "%s: eCryptfs: unrecognized option [%s]\n",
>> -			       __func__, p);
> 
> I think we lost this error message, which can be helpful for users when
> debugging mount(2) failures.

Invalid options never make it to the parsing function under the new mount
API, see below. Honestly we should be able to even remove default: but it seems
like most prior conversions don't do that, maybe to keep static checkers happy,
not sure.

> See below in your new code where we handle the default case for what I
> think we should include.
> 
>> +
>> +	opt = fs_parse(fc, ecryptfs_fs_param_spec, param, &result);
>> +	if (opt < 0)
>> +		return opt;
>> +
>> +	switch (opt) {
>> +	case Opt_sig:
>> +	case Opt_ecryptfs_sig:
>> +		rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
>> +						  param->string, 0);
>> +		if (rc) {
>> +			printk(KERN_ERR "Error attempting to register "
>> +			       "global sig; rc = [%d]\n", rc);
> 
> Are we expected to be using errorf() and friends here rather than
> printk()?

That's kind of a debate. If you'd rather get rid of the kernel message and
send it out through the mount api message channel instead, I can make that
change. But if userspace doesn't capture the message from errorf, that change
would lose the message altogether.

I kind of feel like once userspace is really making use of the message channel,
we could go back and selectively change printks to the message channel where it
makes sense.

>> +			return rc;;
> 
> There's an extra semicolon here.

Ugh, sorry.

>>  		}
>> +		ctx->sig_set = 1;
>> +		break;
>> +	case Opt_cipher:
>> +	case Opt_ecryptfs_cipher:
>> +		strscpy(mount_crypt_stat->global_default_cipher_name,
>> +			param->string);
>> +		ctx->cipher_name_set = 1;
>> +		break;
>> +	case Opt_ecryptfs_key_bytes:
>> +		mount_crypt_stat->global_default_cipher_key_size =
>> +			result.uint_32;
>> +		ctx->cipher_key_bytes_set = 1;
>> +		break;
>> +	case Opt_passthrough:
>> +		mount_crypt_stat->flags |=
>> +			ECRYPTFS_PLAINTEXT_PASSTHROUGH_ENABLED;
>> +		break;
>> +	case Opt_xattr_metadata:
>> +		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
>> +		break;
>> +	case Opt_encrypted_view:
>> +		mount_crypt_stat->flags |= ECRYPTFS_XATTR_METADATA_ENABLED;
>> +		mount_crypt_stat->flags |= ECRYPTFS_ENCRYPTED_VIEW_ENABLED;
>> +		break;
>> +	case Opt_fnek_sig:
>> +		strscpy(mount_crypt_stat->global_default_fnek_sig,
>> +			param->string);
>> +		rc = ecryptfs_add_global_auth_tok(
>> +			mount_crypt_stat,
>> +			mount_crypt_stat->global_default_fnek_sig,
>> +			ECRYPTFS_AUTH_TOK_FNEK);
>> +		if (rc) {
>> +			printk(KERN_ERR "Error attempting to register "
>> +			       "global fnek sig [%s]; rc = [%d]\n",
>> +			       mount_crypt_stat->global_default_fnek_sig, rc);
> 
> Same errorf() question here.

Same answer. :) If you don't mind those messages disappearing from kernel logs
I can change it.

>> +			return rc;
>> +		}
>> +		mount_crypt_stat->flags |=
>> +			(ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES
>> +			 | ECRYPTFS_GLOBAL_ENCFN_USE_MOUNT_FNEK);
>> +		break;
>> +	case Opt_fn_cipher:
>> +		strscpy(mount_crypt_stat->global_default_fn_cipher_name,
>> +			param->string);
>> +		ctx->fn_cipher_name_set = 1;
>> +		break;
>> +	case Opt_fn_cipher_key_bytes:
>> +		mount_crypt_stat->global_default_fn_cipher_key_bytes =
>> +			result.uint_32;
>> +		ctx->fn_cipher_key_bytes_set = 1;
>> +		break;
>> +	case Opt_unlink_sigs:
>> +		mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS;
>> +		break;
>> +	case Opt_mount_auth_tok_only:
>> +		mount_crypt_stat->flags |= ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY;
>> +		break;
>> +	case Opt_check_dev_ruid:
>> +		ctx->check_ruid = 1;
>> +		break;
>> +	default:
> 
> To retain the unrecognized option warning, I think we need this:
> 
> 
>                 printk(KERN_WARNING "%s: eCryptfs: unrecognized option [%s]\n",
>                        __func__, param->key);

I think you'll see that if you make that change, you never hit the default:
case and it's still not printed.

vfs_parse_fs_param() does do "invalf(fc, "%s: Unknown parameter '%s'", ..."
which sends the message back out over the api message interface, and modern util-linux
mount will emit it on the console.

For example (just since I had jfs handy and it's mount is simple) if I add:

        default:
                printk("default yo\n");
                return -EINVAL;

and try it:

# mount -o loop,blahblah jfsfile mnt
mount: /root/jfs-test/mnt: fsconfig system call failed: jfs: Unknown parameter 'blahblah'.
       dmesg(1) may have more information after failed mount system call.
# dmesg | tail -n 2
[61556.764697] JFS: nTxBlock = 8192, nTxLock = 65536
[61561.280700] loop1: detected capacity change from 0 to 204800
#

Thanks for the review, and sorry for the missed nitpicks.
-Eric


> Tyler

Re: [PATCH 2/2] ecryptfs: Convert ecryptfs to use the new mount API

[Eric Sandeen]

On 10/21/24 9:07 AM, Eric Sandeen wrote:

>>> +
>>> +	opt = fs_parse(fc, ecryptfs_fs_param_spec, param, &result);
>>> +	if (opt < 0)
>>> +		return opt;
>>> +
>>> +	switch (opt) {
>>> +	case Opt_sig:
>>> +	case Opt_ecryptfs_sig:
>>> +		rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
>>> +						  param->string, 0);
>>> +		if (rc) {
>>> +			printk(KERN_ERR "Error attempting to register "
>>> +			       "global sig; rc = [%d]\n", rc);
>>
>> Are we expected to be using errorf() and friends here rather than
>> printk()?
> 
> That's kind of a debate. If you'd rather get rid of the kernel message and
> send it out through the mount api message channel instead, I can make that
> change. But if userspace doesn't capture the message from errorf, that change
> would lose the message altogether.
> 
> I kind of feel like once userspace is really making use of the message channel,
> we could go back and selectively change printks to the message channel where it
> makes sense.

Ok, without any further input I'll send V2 w/ the whitespace and double semicolon
fixes, and leave the printks etc in place for the reasons stated above.

If anyone wants to redirect pritnks to the API message channel, that can always
be done as a followup patch. ecryptfs would certainly not be an outlier if it
leaves the printks in place for now.

-Eric

Re: [PATCH 2/2] ecryptfs: Convert ecryptfs to use the new mount API

[Tyler Hicks]

On 2024-10-28 09:22:35, Eric Sandeen wrote:
> On 10/21/24 9:07 AM, Eric Sandeen wrote:
> 
> >>> +
> >>> +	opt = fs_parse(fc, ecryptfs_fs_param_spec, param, &result);
> >>> +	if (opt < 0)
> >>> +		return opt;
> >>> +
> >>> +	switch (opt) {
> >>> +	case Opt_sig:
> >>> +	case Opt_ecryptfs_sig:
> >>> +		rc = ecryptfs_add_global_auth_tok(mount_crypt_stat,
> >>> +						  param->string, 0);
> >>> +		if (rc) {
> >>> +			printk(KERN_ERR "Error attempting to register "
> >>> +			       "global sig; rc = [%d]\n", rc);
> >>
> >> Are we expected to be using errorf() and friends here rather than
> >> printk()?
> > 
> > That's kind of a debate. If you'd rather get rid of the kernel message and
> > send it out through the mount api message channel instead, I can make that
> > change. But if userspace doesn't capture the message from errorf, that change
> > would lose the message altogether.
> > 
> > I kind of feel like once userspace is really making use of the message channel,
> > we could go back and selectively change printks to the message channel where it
> > makes sense.
> 
> Ok, without any further input I'll send V2 w/ the whitespace and double semicolon
> fixes, and leave the printks etc in place for the reasons stated above.

Apologies. I agree that this was the correct approach for v2.

> If anyone wants to redirect pritnks to the API message channel, that can always
> be done as a followup patch. ecryptfs would certainly not be an outlier if it
> leaves the printks in place for now.

Thanks for that info. I'll review v2 shortly.

Tyler

> 
> -Eric
>