[PATCH 0/14] New uid & gid mount option parsing helpers

[PATCH 0/14] New uid & gid mount option parsing helpers

[Eric Sandeen]

Multiple filesystems take uid and gid as options, and the code to
create the ID from an integer and validate it is standard boilerplate
that can be moved into common helper functions, so do that for
consistency and less cut&paste.

This also helps avoid the buggy pattern noted by Seth Jenkins at
https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
because uid/gid parsing will fail before any assignment in most
filesystems.

Net effect is a bit of code removal, as well.

Patch 1 is the infrastructure change, then per-fs conversions follow,
cc'd as appropriate.

This series is also at
https://git.kernel.org/pub/scm/linux/kernel/git/sandeen/linux.git/log/?h=mount-api-uid-helper

Thanks,
-Eric

 Documentation/filesystems/mount_api.rst |    9 +++++++--
 fs/autofs/inode.c                       |   16 ++++------------
 fs/debugfs/inode.c                      |   16 ++++------------
 fs/efivarfs/super.c                     |   12 ++++--------
 fs/exfat/super.c                        |    8 ++++----
 fs/ext4/super.c                         |   22 ++++------------------
 fs/fs_parser.c                          |   34 ++++++++++++++++++++++++++++++++++
 fs/fuse/inode.c                         |   12 ++++--------
 fs/hugetlbfs/inode.c                    |   12 ++++--------
 fs/isofs/inode.c                        |   16 ++++------------
 fs/ntfs3/super.c                        |   12 ++++--------
 fs/smb/client/fs_context.c              |   39 ++++++++++++---------------------------
 fs/tracefs/inode.c                      |   16 ++++------------
 include/linux/fs_parser.h               |    6 +++++-
 mm/shmem.c                              |   12 ++++--------
 15 files changed, 102 insertions(+), 140 deletions(-)

[PATCH 01/14] fs_parse: add uid & gid option option parsing helpers

[Eric Sandeen]

Multiple filesystems take uid and gid as options, and the code to
create the ID from an integer and validate it is standard boilerplate
that can be moved into common helper functions, so do that for
consistency and less cut&paste.

This also helps avoid the buggy pattern noted by Seth Jenkins at
https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
because uid/gid parsing will fail before any assignment in most
filesystems.

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---
 Documentation/filesystems/mount_api.rst |  9 +++++--
 fs/fs_parser.c                          | 34 +++++++++++++++++++++++++
 include/linux/fs_parser.h               |  6 ++++-
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst
index 9aaf6ef75eb5..317934c9e8fc 100644
--- a/Documentation/filesystems/mount_api.rst
+++ b/Documentation/filesystems/mount_api.rst
@@ -645,6 +645,8 @@ The members are as follows:
 	fs_param_is_blockdev	Blockdev path		* Needs lookup
 	fs_param_is_path	Path			* Needs lookup
 	fs_param_is_fd		File descriptor		result->int_32
+	fs_param_is_uid		User ID (u32)           result->uid
+	fs_param_is_gid		Group ID (u32)          result->gid
 	=======================	=======================	=====================
 
      Note that if the value is of fs_param_is_bool type, fs_parse() will try
@@ -678,6 +680,8 @@ The members are as follows:
 	fsparam_bdev()		fs_param_is_blockdev
 	fsparam_path()		fs_param_is_path
 	fsparam_fd()		fs_param_is_fd
+	fsparam_uid()		fs_param_is_uid
+	fsparam_gid()		fs_param_is_gid
 	=======================	===============================================
 
      all of which take two arguments, name string and option number - for
@@ -784,8 +788,9 @@ process the parameters it is given.
      option number (which it returns).
 
      If successful, and if the parameter type indicates the result is a
-     boolean, integer or enum type, the value is converted by this function and
-     the result stored in result->{boolean,int_32,uint_32,uint_64}.
+     boolean, integer, enum, uid, or gid type, the value is converted by this
+     function and the result stored in
+     result->{boolean,int_32,uint_32,uint_64,uid,gid}.
 
      If a match isn't initially made, the key is prefixed with "no" and no
      value is present then an attempt will be made to look up the key with the
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index a4d6ca0b8971..24727ec34e5a 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
 }
 EXPORT_SYMBOL(fs_param_is_fd);
 
+int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
+		    struct fs_parameter *param, struct fs_parse_result *result)
+{
+	kuid_t uid;
+
+	if (fs_param_is_u32(log, p, param, result) != 0)
+		return fs_param_bad_value(log, param);
+
+	uid = make_kuid(current_user_ns(), result->uint_32);
+	if (!uid_valid(uid))
+		return inval_plog(log, "Invalid uid '%s'", param->string);
+
+	result->uid = uid;
+	return 0;
+}
+EXPORT_SYMBOL(fs_param_is_uid);
+
+int fs_param_is_gid(struct p_log *log, const struct fs_parameter_spec *p,
+		    struct fs_parameter *param, struct fs_parse_result *result)
+{
+	kgid_t gid;
+
+	if (fs_param_is_u32(log, p, param, result) != 0)
+		return fs_param_bad_value(log, param);
+
+	gid = make_kgid(current_user_ns(), result->uint_32);
+	if (!gid_valid(gid))
+		return inval_plog(log, "Invalid gid '%s'", param->string);
+
+	result->gid = gid;
+	return 0;
+}
+EXPORT_SYMBOL(fs_param_is_gid);
+
 int fs_param_is_blockdev(struct p_log *log, const struct fs_parameter_spec *p,
 		  struct fs_parameter *param, struct fs_parse_result *result)
 {
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index d3350979115f..6cf713a7e6c6 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -28,7 +28,7 @@ typedef int fs_param_type(struct p_log *,
  */
 fs_param_type fs_param_is_bool, fs_param_is_u32, fs_param_is_s32, fs_param_is_u64,
 	fs_param_is_enum, fs_param_is_string, fs_param_is_blob, fs_param_is_blockdev,
-	fs_param_is_path, fs_param_is_fd;
+	fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid;
 
 /*
  * Specification of the type of value a parameter wants.
@@ -57,6 +57,8 @@ struct fs_parse_result {
 		int		int_32;		/* For spec_s32/spec_enum */
 		unsigned int	uint_32;	/* For spec_u32{,_octal,_hex}/spec_enum */
 		u64		uint_64;	/* For spec_u64 */
+		kuid_t		uid;
+		kgid_t		gid;
 	};
 };
 
@@ -131,6 +133,8 @@ static inline bool fs_validate_description(const char *name,
 #define fsparam_bdev(NAME, OPT)	__fsparam(fs_param_is_blockdev, NAME, OPT, 0, NULL)
 #define fsparam_path(NAME, OPT)	__fsparam(fs_param_is_path, NAME, OPT, 0, NULL)
 #define fsparam_fd(NAME, OPT)	__fsparam(fs_param_is_fd, NAME, OPT, 0, NULL)
+#define fsparam_uid(NAME, OPT) __fsparam(fs_param_is_uid, NAME, OPT, 0, NULL)
+#define fsparam_gid(NAME, OPT) __fsparam(fs_param_is_gid, NAME, OPT, 0, NULL)
 
 /* String parameter that allows empty argument */
 #define fsparam_string_empty(NAME, OPT) \
-- 
2.45.2

[PATCH 02/14] autofs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---
 fs/autofs/inode.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index 1f5db6863663..cf792d4de4f1 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -126,7 +126,7 @@ enum {
 const struct fs_parameter_spec autofs_param_specs[] = {
 	fsparam_flag	("direct",		Opt_direct),
 	fsparam_fd	("fd",			Opt_fd),
-	fsparam_u32	("gid",			Opt_gid),
+	fsparam_gid	("gid",			Opt_gid),
 	fsparam_flag	("ignore",		Opt_ignore),
 	fsparam_flag	("indirect",		Opt_indirect),
 	fsparam_u32	("maxproto",		Opt_maxproto),
@@ -134,7 +134,7 @@ const struct fs_parameter_spec autofs_param_specs[] = {
 	fsparam_flag	("offset",		Opt_offset),
 	fsparam_u32	("pgrp",		Opt_pgrp),
 	fsparam_flag	("strictexpire",	Opt_strictexpire),
-	fsparam_u32	("uid",			Opt_uid),
+	fsparam_uid	("uid",			Opt_uid),
 	{}
 };
 
@@ -193,8 +193,6 @@ static int autofs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	struct autofs_fs_context *ctx = fc->fs_private;
 	struct autofs_sb_info *sbi = fc->s_fs_info;
 	struct fs_parse_result result;
-	kuid_t uid;
-	kgid_t gid;
 	int opt;
 
 	opt = fs_parse(fc, autofs_param_specs, param, &result);
@@ -205,16 +203,10 @@ static int autofs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case Opt_fd:
 		return autofs_parse_fd(fc, sbi, param, &result);
 	case Opt_uid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			return invalfc(fc, "Invalid uid");
-		ctx->uid = uid;
+		ctx->uid = result.uid;
 		break;
 	case Opt_gid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			return invalfc(fc, "Invalid gid");
-		ctx->gid = gid;
+		ctx->gid = result.gid;
 		break;
 	case Opt_pgrp:
 		ctx->pgrp = result.uint_32;
-- 
2.45.2

[PATCH 03/14] debugfs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/debugfs/inode.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 8fd928899a59..91521576f500 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -92,9 +92,9 @@ enum {
 };
 
 static const struct fs_parameter_spec debugfs_param_specs[] = {
-	fsparam_u32	("gid",		Opt_gid),
+	fsparam_gid	("gid",		Opt_gid),
 	fsparam_u32oct	("mode",	Opt_mode),
-	fsparam_u32	("uid",		Opt_uid),
+	fsparam_uid	("uid",		Opt_uid),
 	{}
 };
 
@@ -102,8 +102,6 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param
 {
 	struct debugfs_fs_info *opts = fc->s_fs_info;
 	struct fs_parse_result result;
-	kuid_t uid;
-	kgid_t gid;
 	int opt;
 
 	opt = fs_parse(fc, debugfs_param_specs, param, &result);
@@ -120,16 +118,10 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param
 
 	switch (opt) {
 	case Opt_uid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			return invalf(fc, "Unknown uid");
-		opts->uid = uid;
+		opts->uid = result.uid;
 		break;
 	case Opt_gid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			return invalf(fc, "Unknown gid");
-		opts->gid = gid;
+		opts->gid = result.gid;
 		break;
 	case Opt_mode:
 		opts->mode = result.uint_32 & S_IALLUGO;
-- 
2.45.2

[PATCH 04/14] efivarfs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/efivarfs/super.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index bb14462f6d99..a929f1b613be 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -275,8 +275,8 @@ enum {
 };
 
 static const struct fs_parameter_spec efivarfs_parameters[] = {
-	fsparam_u32("uid", Opt_uid),
-	fsparam_u32("gid", Opt_gid),
+	fsparam_uid("uid", Opt_uid),
+	fsparam_gid("gid", Opt_gid),
 	{},
 };
 
@@ -293,14 +293,10 @@ static int efivarfs_parse_param(struct fs_context *fc, struct fs_parameter *para
 
 	switch (opt) {
 	case Opt_uid:
-		opts->uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(opts->uid))
-			return -EINVAL;
+		opts->uid = result.uid;
 		break;
 	case Opt_gid:
-		opts->gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(opts->gid))
-			return -EINVAL;
+		opts->gid = result.gid;
 		break;
 	default:
 		return -EINVAL;
-- 
2.45.2

[PATCH 05/14] exfat: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/exfat/super.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 3d5ea2cfad66..a3c7173ef693 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -225,8 +225,8 @@ static const struct constant_table exfat_param_enums[] = {
 };
 
 static const struct fs_parameter_spec exfat_parameters[] = {
-	fsparam_u32("uid",			Opt_uid),
-	fsparam_u32("gid",			Opt_gid),
+	fsparam_uid("uid",			Opt_uid),
+	fsparam_gid("gid",			Opt_gid),
 	fsparam_u32oct("umask",			Opt_umask),
 	fsparam_u32oct("dmask",			Opt_dmask),
 	fsparam_u32oct("fmask",			Opt_fmask),
@@ -262,10 +262,10 @@ static int exfat_parse_param(struct fs_context *fc, struct fs_parameter *param)
 
 	switch (opt) {
 	case Opt_uid:
-		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
+		opts->fs_uid = result.uid;
 		break;
 	case Opt_gid:
-		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
+		opts->fs_gid = result.gid;
 		break;
 	case Opt_umask:
 		opts->fs_fmask = result.uint_32;
-- 
2.45.2

[PATCH 06/14] ext4: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/ext4/super.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c682fb927b64..0c614df3225d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1721,8 +1721,8 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
 	fsparam_flag	("bsdgroups",		Opt_grpid),
 	fsparam_flag	("nogrpid",		Opt_nogrpid),
 	fsparam_flag	("sysvgroups",		Opt_nogrpid),
-	fsparam_u32	("resgid",		Opt_resgid),
-	fsparam_u32	("resuid",		Opt_resuid),
+	fsparam_gid	("resgid",		Opt_resgid),
+	fsparam_uid	("resuid",		Opt_resuid),
 	fsparam_u32	("sb",			Opt_sb),
 	fsparam_enum	("errors",		Opt_errors, ext4_param_errors),
 	fsparam_flag	("nouid32",		Opt_nouid32),
@@ -2127,8 +2127,6 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	struct fs_parse_result result;
 	const struct mount_opts *m;
 	int is_remount;
-	kuid_t uid;
-	kgid_t gid;
 	int token;
 
 	token = fs_parse(fc, ext4_param_specs, param, &result);
@@ -2270,23 +2268,11 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		ctx->spec |= EXT4_SPEC_s_stripe;
 		return 0;
 	case Opt_resuid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid)) {
-			ext4_msg(NULL, KERN_ERR, "Invalid uid value %d",
-				 result.uint_32);
-			return -EINVAL;
-		}
-		ctx->s_resuid = uid;
+		ctx->s_resuid = result.uid;
 		ctx->spec |= EXT4_SPEC_s_resuid;
 		return 0;
 	case Opt_resgid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid)) {
-			ext4_msg(NULL, KERN_ERR, "Invalid gid value %d",
-				 result.uint_32);
-			return -EINVAL;
-		}
-		ctx->s_resgid = gid;
+		ctx->s_resgid = result.gid;
 		ctx->spec |= EXT4_SPEC_s_resgid;
 		return 0;
 	case Opt_journal_dev:
-- 
2.45.2

[PATCH 07/14] fuse: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/fuse/inode.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 99e44ea7d875..1ac528bcdb3c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -740,8 +740,8 @@ static const struct fs_parameter_spec fuse_fs_parameters[] = {
 	fsparam_string	("source",		OPT_SOURCE),
 	fsparam_u32	("fd",			OPT_FD),
 	fsparam_u32oct	("rootmode",		OPT_ROOTMODE),
-	fsparam_u32	("user_id",		OPT_USER_ID),
-	fsparam_u32	("group_id",		OPT_GROUP_ID),
+	fsparam_uid	("user_id",		OPT_USER_ID),
+	fsparam_gid	("group_id",		OPT_GROUP_ID),
 	fsparam_flag	("default_permissions",	OPT_DEFAULT_PERMISSIONS),
 	fsparam_flag	("allow_other",		OPT_ALLOW_OTHER),
 	fsparam_u32	("max_read",		OPT_MAX_READ),
@@ -799,16 +799,12 @@ static int fuse_parse_param(struct fs_context *fsc, struct fs_parameter *param)
 		break;
 
 	case OPT_USER_ID:
-		ctx->user_id = make_kuid(fsc->user_ns, result.uint_32);
-		if (!uid_valid(ctx->user_id))
-			return invalfc(fsc, "Invalid user_id");
+		ctx->user_id = result.uid;
 		ctx->user_id_present = true;
 		break;
 
 	case OPT_GROUP_ID:
-		ctx->group_id = make_kgid(fsc->user_ns, result.uint_32);
-		if (!gid_valid(ctx->group_id))
-			return invalfc(fsc, "Invalid group_id");
+		ctx->group_id = result.gid;
 		ctx->group_id_present = true;
 		break;
 
-- 
2.45.2

[PATCH 08/14] hugetlbfs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/hugetlbfs/inode.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 412f295acebe..81dab95f67ed 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -73,13 +73,13 @@ enum hugetlb_param {
 };
 
 static const struct fs_parameter_spec hugetlb_fs_parameters[] = {
-	fsparam_u32   ("gid",		Opt_gid),
+	fsparam_gid   ("gid",		Opt_gid),
 	fsparam_string("min_size",	Opt_min_size),
 	fsparam_u32oct("mode",		Opt_mode),
 	fsparam_string("nr_inodes",	Opt_nr_inodes),
 	fsparam_string("pagesize",	Opt_pagesize),
 	fsparam_string("size",		Opt_size),
-	fsparam_u32   ("uid",		Opt_uid),
+	fsparam_uid   ("uid",		Opt_uid),
 	{}
 };
 
@@ -1376,15 +1376,11 @@ static int hugetlbfs_parse_param(struct fs_context *fc, struct fs_parameter *par
 
 	switch (opt) {
 	case Opt_uid:
-		ctx->uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(ctx->uid))
-			goto bad_val;
+		ctx->uid = result.uid;
 		return 0;
 
 	case Opt_gid:
-		ctx->gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(ctx->gid))
-			goto bad_val;
+		ctx->gid = result.gid;
 		return 0;
 
 	case Opt_mode:
-- 
2.45.2

[PATCH 09/14] isofs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/isofs/inode.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 93b1077a380a..ed548efdd9bb 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -326,8 +326,8 @@ static const struct fs_parameter_spec isofs_param_spec[] = {
 	fsparam_u32	("session",		Opt_session),
 	fsparam_u32	("sbsector",		Opt_sb),
 	fsparam_enum	("check",		Opt_check, isofs_param_check),
-	fsparam_u32	("uid",			Opt_uid),
-	fsparam_u32	("gid",			Opt_gid),
+	fsparam_uid	("uid",			Opt_uid),
+	fsparam_gid	("gid",			Opt_gid),
 	/* Note: mode/dmode historically accepted %u not strictly %o */
 	fsparam_u32	("mode",		Opt_mode),
 	fsparam_u32	("dmode",		Opt_dmode),
@@ -344,8 +344,6 @@ static int isofs_parse_param(struct fs_context *fc,
 	struct isofs_options *popt = fc->fs_private;
 	struct fs_parse_result result;
 	int opt;
-	kuid_t uid;
-	kgid_t gid;
 	unsigned int n;
 
 	/* There are no remountable options */
@@ -409,17 +407,11 @@ static int isofs_parse_param(struct fs_context *fc,
 	case Opt_ignore:
 		break;
 	case Opt_uid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			return -EINVAL;
-		popt->uid = uid;
+		popt->uid = result.uid;
 		popt->uid_set = 1;
 		break;
 	case Opt_gid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			return -EINVAL;
-		popt->gid = gid;
+		popt->gid = result.gid;
 		popt->gid_set = 1;
 		break;
 	case Opt_mode:
-- 
2.45.2

[PATCH 10/14] ntfs3: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/ntfs3/super.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 27fbde2701b6..c5b688c5f984 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -259,8 +259,8 @@ enum Opt {
 
 // clang-format off
 static const struct fs_parameter_spec ntfs_fs_parameters[] = {
-	fsparam_u32("uid",			Opt_uid),
-	fsparam_u32("gid",			Opt_gid),
+	fsparam_uid("uid",			Opt_uid),
+	fsparam_gid("gid",			Opt_gid),
 	fsparam_u32oct("umask",			Opt_umask),
 	fsparam_u32oct("dmask",			Opt_dmask),
 	fsparam_u32oct("fmask",			Opt_fmask),
@@ -319,14 +319,10 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
 
 	switch (opt) {
 	case Opt_uid:
-		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(opts->fs_uid))
-			return invalf(fc, "ntfs3: Invalid value for uid.");
+		opts->fs_uid = result.uid;
 		break;
 	case Opt_gid:
-		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(opts->fs_gid))
-			return invalf(fc, "ntfs3: Invalid value for gid.");
+		opts->fs_gid = result.gid;
 		break;
 	case Opt_umask:
 		if (result.uint_32 & ~07777)
-- 
2.45.2

[PATCH 11/14] tmpfs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 mm/shmem.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index a8b181a63402..922204184a82 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3903,14 +3903,14 @@ static const struct constant_table shmem_param_enums_huge[] = {
 };
 
 const struct fs_parameter_spec shmem_fs_parameters[] = {
-	fsparam_u32   ("gid",		Opt_gid),
+	fsparam_gid   ("gid",		Opt_gid),
 	fsparam_enum  ("huge",		Opt_huge,  shmem_param_enums_huge),
 	fsparam_u32oct("mode",		Opt_mode),
 	fsparam_string("mpol",		Opt_mpol),
 	fsparam_string("nr_blocks",	Opt_nr_blocks),
 	fsparam_string("nr_inodes",	Opt_nr_inodes),
 	fsparam_string("size",		Opt_size),
-	fsparam_u32   ("uid",		Opt_uid),
+	fsparam_uid   ("uid",		Opt_uid),
 	fsparam_flag  ("inode32",	Opt_inode32),
 	fsparam_flag  ("inode64",	Opt_inode64),
 	fsparam_flag  ("noswap",	Opt_noswap),
@@ -3970,9 +3970,7 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 		ctx->mode = result.uint_32 & 07777;
 		break;
 	case Opt_uid:
-		kuid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(kuid))
-			goto bad_value;
+		kuid = result.uid;
 
 		/*
 		 * The requested uid must be representable in the
@@ -3984,9 +3982,7 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param)
 		ctx->uid = kuid;
 		break;
 	case Opt_gid:
-		kgid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(kgid))
-			goto bad_value;
+		kgid = result.gid;
 
 		/*
 		 * The requested gid must be representable in the
-- 
2.45.2

[PATCH 12/14] smb: client: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/smb/client/fs_context.c | 39 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 3bbac925d076..bc926ab2555b 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -128,12 +128,14 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_flag("compress", Opt_compress),
 	fsparam_flag("witness", Opt_witness),
 
+	/* Mount options which take uid or gid */
+	fsparam_uid("backupuid", Opt_backupuid),
+	fsparam_gid("backupgid", Opt_backupgid),
+	fsparam_uid("uid", Opt_uid),
+	fsparam_uid("cruid", Opt_cruid),
+	fsparam_gid("gid", Opt_gid),
+
 	/* Mount options which take numeric value */
-	fsparam_u32("backupuid", Opt_backupuid),
-	fsparam_u32("backupgid", Opt_backupgid),
-	fsparam_u32("uid", Opt_uid),
-	fsparam_u32("cruid", Opt_cruid),
-	fsparam_u32("gid", Opt_gid),
 	fsparam_u32("file_mode", Opt_file_mode),
 	fsparam_u32("dirmode", Opt_dirmode),
 	fsparam_u32("dir_mode", Opt_dirmode),
@@ -951,8 +953,6 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 	int i, opt;
 	bool is_smb3 = !strcmp(fc->fs_type->name, "smb3");
 	bool skip_parsing = false;
-	kuid_t uid;
-	kgid_t gid;
 
 	cifs_dbg(FYI, "CIFS: parsing cifs mount option '%s'\n", param->key);
 
@@ -1083,38 +1083,23 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		}
 		break;
 	case Opt_uid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			goto cifs_parse_mount_err;
-		ctx->linux_uid = uid;
+		ctx->linux_uid = result.uid;
 		ctx->uid_specified = true;
 		break;
 	case Opt_cruid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			goto cifs_parse_mount_err;
-		ctx->cred_uid = uid;
+		ctx->cred_uid = result.uid;
 		ctx->cruid_specified = true;
 		break;
 	case Opt_backupuid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			goto cifs_parse_mount_err;
-		ctx->backupuid = uid;
+		ctx->backupuid = result.uid;
 		ctx->backupuid_specified = true;
 		break;
 	case Opt_backupgid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			goto cifs_parse_mount_err;
-		ctx->backupgid = gid;
+		ctx->backupgid = result.gid;
 		ctx->backupgid_specified = true;
 		break;
 	case Opt_gid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			goto cifs_parse_mount_err;
-		ctx->linux_gid = gid;
+		ctx->linux_gid = result.gid;
 		ctx->gid_specified = true;
 		break;
 	case Opt_port:
-- 
2.45.2

[PATCH 13/14] tracefs: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/tracefs/inode.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 7c29f4afc23d..1028ab6d9a74 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -296,9 +296,9 @@ enum {
 };
 
 static const struct fs_parameter_spec tracefs_param_specs[] = {
-	fsparam_u32	("gid",		Opt_gid),
+	fsparam_gid	("gid",		Opt_gid),
 	fsparam_u32oct	("mode",	Opt_mode),
-	fsparam_u32	("uid",		Opt_uid),
+	fsparam_uid	("uid",		Opt_uid),
 	{}
 };
 
@@ -306,8 +306,6 @@ static int tracefs_parse_param(struct fs_context *fc, struct fs_parameter *param
 {
 	struct tracefs_fs_info *opts = fc->s_fs_info;
 	struct fs_parse_result result;
-	kuid_t uid;
-	kgid_t gid;
 	int opt;
 
 	opt = fs_parse(fc, tracefs_param_specs, param, &result);
@@ -316,16 +314,10 @@ static int tracefs_parse_param(struct fs_context *fc, struct fs_parameter *param
 
 	switch (opt) {
 	case Opt_uid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			return invalf(fc, "Unknown uid");
-		opts->uid = uid;
+		opts->uid = result.uid;
 		break;
 	case Opt_gid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			return invalf(fc, "Unknown gid");
-		opts->gid = gid;
+		opts->gid = result.gid;
 		break;
 	case Opt_mode:
 		opts->mode = result.uint_32 & S_IALLUGO;
-- 
2.45.2

[PATCH 14/14] vboxsf: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

Convert to new uid/gid option parsing helpers

From: Eric Sandeen <sandeen@redhat.com>

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/vboxsf/super.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
index ffb1d565da39..e95b8a48d8a0 100644
--- a/fs/vboxsf/super.c
+++ b/fs/vboxsf/super.c
@@ -41,8 +41,8 @@ enum  { opt_nls, opt_uid, opt_gid, opt_ttl, opt_dmode, opt_fmode,
 
 static const struct fs_parameter_spec vboxsf_fs_parameters[] = {
 	fsparam_string	("nls",		opt_nls),
-	fsparam_u32	("uid",		opt_uid),
-	fsparam_u32	("gid",		opt_gid),
+	fsparam_uid	("uid",		opt_uid),
+	fsparam_gid	("gid",		opt_gid),
 	fsparam_u32	("ttl",		opt_ttl),
 	fsparam_u32oct	("dmode",	opt_dmode),
 	fsparam_u32oct	("fmode",	opt_fmode),
@@ -55,8 +55,6 @@ static int vboxsf_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
 	struct vboxsf_fs_context *ctx = fc->fs_private;
 	struct fs_parse_result result;
-	kuid_t uid;
-	kgid_t gid;
 	int opt;
 
 	opt = fs_parse(fc, vboxsf_fs_parameters, param, &result);
@@ -73,16 +71,10 @@ static int vboxsf_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		param->string = NULL;
 		break;
 	case opt_uid:
-		uid = make_kuid(current_user_ns(), result.uint_32);
-		if (!uid_valid(uid))
-			return -EINVAL;
-		ctx->o.uid = uid;
+		ctx->o.uid = result.uid;
 		break;
 	case opt_gid:
-		gid = make_kgid(current_user_ns(), result.uint_32);
-		if (!gid_valid(gid))
-			return -EINVAL;
-		ctx->o.gid = gid;
+		ctx->o.gid = result.gid;
 		break;
 	case opt_ttl:
 		ctx->o.ttl = msecs_to_jiffies(result.uint_32);
-- 
2.45.2

Re: [PATCH 14/14] vboxsf: Convert to new uid/gid option parsing helpers

[Hans de Goede]

Hi,

On 6/28/24 2:42 AM, Eric Sandeen wrote:
> Convert to new uid/gid option parsing helpers
> 
> From: Eric Sandeen <sandeen@redhat.com>
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Please feel free to take this upstream through whatever
tree/branch is convenient.

Regards,

Hans

> ---
>  fs/vboxsf/super.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
> index ffb1d565da39..e95b8a48d8a0 100644
> --- a/fs/vboxsf/super.c
> +++ b/fs/vboxsf/super.c
> @@ -41,8 +41,8 @@ enum  { opt_nls, opt_uid, opt_gid, opt_ttl, opt_dmode, opt_fmode,
>  
>  static const struct fs_parameter_spec vboxsf_fs_parameters[] = {
>  	fsparam_string	("nls",		opt_nls),
> -	fsparam_u32	("uid",		opt_uid),
> -	fsparam_u32	("gid",		opt_gid),
> +	fsparam_uid	("uid",		opt_uid),
> +	fsparam_gid	("gid",		opt_gid),
>  	fsparam_u32	("ttl",		opt_ttl),
>  	fsparam_u32oct	("dmode",	opt_dmode),
>  	fsparam_u32oct	("fmode",	opt_fmode),
> @@ -55,8 +55,6 @@ static int vboxsf_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  {
>  	struct vboxsf_fs_context *ctx = fc->fs_private;
>  	struct fs_parse_result result;
> -	kuid_t uid;
> -	kgid_t gid;
>  	int opt;
>  
>  	opt = fs_parse(fc, vboxsf_fs_parameters, param, &result);
> @@ -73,16 +71,10 @@ static int vboxsf_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		param->string = NULL;
>  		break;
>  	case opt_uid:
> -		uid = make_kuid(current_user_ns(), result.uint_32);
> -		if (!uid_valid(uid))
> -			return -EINVAL;
> -		ctx->o.uid = uid;
> +		ctx->o.uid = result.uid;
>  		break;
>  	case opt_gid:
> -		gid = make_kgid(current_user_ns(), result.uint_32);
> -		if (!gid_valid(gid))
> -			return -EINVAL;
> -		ctx->o.gid = gid;
> +		ctx->o.gid = result.gid;
>  		break;
>  	case opt_ttl:
>  		ctx->o.ttl = msecs_to_jiffies(result.uint_32);

Re: [PATCH 01/14] fs_parse: add uid & gid option option parsing helpers

[Jan Kara]

On Thu 27-06-24 19:26:24, Eric Sandeen wrote:
> Multiple filesystems take uid and gid as options, and the code to
> create the ID from an integer and validate it is standard boilerplate
> that can be moved into common helper functions, so do that for
> consistency and less cut&paste.
> 
> This also helps avoid the buggy pattern noted by Seth Jenkins at
> https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
> because uid/gid parsing will fail before any assignment in most
> filesystems.
> 
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

I like the idea since this seems like a nobrainer but is actually
surprisingly subtle...

> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index a4d6ca0b8971..24727ec34e5a 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
>  }
>  EXPORT_SYMBOL(fs_param_is_fd);
>  
> +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
> +		    struct fs_parameter *param, struct fs_parse_result *result)
> +{
> +	kuid_t uid;
> +
> +	if (fs_param_is_u32(log, p, param, result) != 0)
> +		return fs_param_bad_value(log, param);
> +
> +	uid = make_kuid(current_user_ns(), result->uint_32);

But here is the problem: Filesystems mountable in user namespaces need to use
fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()).
Having helpers that work for some filesystems and are subtly broken for
others is worse than no helpers... Or am I missing something?

And the problem with fc->user_ns is that currently __fs_parse() does not
get fs_context as an argument... So that will need some larger work.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/14] New uid & gid mount option parsing helpers

[Christian Brauner]

On Thu, Jun 27, 2024 at 07:24:59PM GMT, Eric Sandeen wrote:
> Multiple filesystems take uid and gid as options, and the code to
> create the ID from an integer and validate it is standard boilerplate
> that can be moved into common helper functions, so do that for
> consistency and less cut&paste.
> 
> This also helps avoid the buggy pattern noted by Seth Jenkins at
> https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
> because uid/gid parsing will fail before any assignment in most
> filesystems.
> 
> Net effect is a bit of code removal, as well.

Thanks, this all looks good to me. I'll have one comment about the fuse
patch.

Re: [PATCH 07/14] fuse: Convert to new uid/gid option parsing helpers

[Christian Brauner]

I think you accidently Cced the wrong Miklos. :)

On Thu, Jun 27, 2024 at 07:33:43PM GMT, Eric Sandeen wrote:
> Convert to new uid/gid option parsing helpers
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/fuse/inode.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 99e44ea7d875..1ac528bcdb3c 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -740,8 +740,8 @@ static const struct fs_parameter_spec fuse_fs_parameters[] = {
>  	fsparam_string	("source",		OPT_SOURCE),
>  	fsparam_u32	("fd",			OPT_FD),
>  	fsparam_u32oct	("rootmode",		OPT_ROOTMODE),
> -	fsparam_u32	("user_id",		OPT_USER_ID),
> -	fsparam_u32	("group_id",		OPT_GROUP_ID),
> +	fsparam_uid	("user_id",		OPT_USER_ID),
> +	fsparam_gid	("group_id",		OPT_GROUP_ID),
>  	fsparam_flag	("default_permissions",	OPT_DEFAULT_PERMISSIONS),
>  	fsparam_flag	("allow_other",		OPT_ALLOW_OTHER),
>  	fsparam_u32	("max_read",		OPT_MAX_READ),
> @@ -799,16 +799,12 @@ static int fuse_parse_param(struct fs_context *fsc, struct fs_parameter *param)
>  		break;
>  
>  	case OPT_USER_ID:
> -		ctx->user_id = make_kuid(fsc->user_ns, result.uint_32);
> -		if (!uid_valid(ctx->user_id))
> -			return invalfc(fsc, "Invalid user_id");
> +		ctx->user_id = result.uid;

So fsc->user_ns will record the namespaces at fsopen() time which can be
different from the namespace used at fsconfig() time. This was done when
fuse was ported to the new mount api.

It has the same potential issues that Seth pointed out so I think your
patch is correct. But I also think we might need the same verification
that tmpfs is doing to verify that the uid/gid we're using can actually
be represented in the fsc->user_ns.

So maybe there should be a separate patch that converts fuse to rely on
make_k*id(current_user_ns()) + k*id_has_mapping() and then these patches
on top?

Re: [PATCH 01/14] fs_parse: add uid & gid option option parsing helpers

[Christian Brauner]

On Fri, Jun 28, 2024 at 11:45:17AM GMT, Jan Kara wrote:
> On Thu 27-06-24 19:26:24, Eric Sandeen wrote:
> > Multiple filesystems take uid and gid as options, and the code to
> > create the ID from an integer and validate it is standard boilerplate
> > that can be moved into common helper functions, so do that for
> > consistency and less cut&paste.
> > 
> > This also helps avoid the buggy pattern noted by Seth Jenkins at
> > https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
> > because uid/gid parsing will fail before any assignment in most
> > filesystems.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> 
> I like the idea since this seems like a nobrainer but is actually
> surprisingly subtle...
> 
> > diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > index a4d6ca0b8971..24727ec34e5a 100644
> > --- a/fs/fs_parser.c
> > +++ b/fs/fs_parser.c
> > @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
> >  }
> >  EXPORT_SYMBOL(fs_param_is_fd);
> >  
> > +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
> > +		    struct fs_parameter *param, struct fs_parse_result *result)
> > +{
> > +	kuid_t uid;
> > +
> > +	if (fs_param_is_u32(log, p, param, result) != 0)
> > +		return fs_param_bad_value(log, param);
> > +
> > +	uid = make_kuid(current_user_ns(), result->uint_32);
> 
> But here is the problem: Filesystems mountable in user namespaces need to use
> fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()).
> Having helpers that work for some filesystems and are subtly broken for
> others is worse than no helpers... Or am I missing something?
> 
> And the problem with fc->user_ns is that currently __fs_parse() does not
> get fs_context as an argument... So that will need some larger work.

Not really. If someone does an fsopen() in a namespace but the process
that actually sets mount options is in another namespace then it's
completely intransparent what uid/gid this will resolve to if it's
resovled according to fsopen().

It's also a bit strange if someone ends up handing off a tmpfs fscontext
that was created in the initial namespace to some random namespace and
they now can set uid/gid options that aren't mapped according to their
namespace but instead are 1:1 resolved according to the intial
namespace. So this would hinder delegation.

The expectation is that uid/gid options are resolved in the caller's
namespace and that shouldn't be any different for fscontexts for
namespace mountable filesystems. The crucial point is to ensure that the
resulting kuid/kgid can be resolved in the namespace the filesystem is
mounted in at the end. That's what was lacking in e.g., tmpfs in commit
0200679fc795 ("tmpfs: verify {g,u}id mount options correctly")

The fuse conversion is the only inconsistency in that regard.

Re: [PATCH 13/14] tracefs: Convert to new uid/gid option parsing helpers

[Steven Rostedt]

On Thu, 27 Jun 2024 19:40:44 -0500
Eric Sandeen <sandeen@redhat.com> wrote:

> Convert to new uid/gid option parsing helpers
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

Re: [PATCH 07/14] fuse: Convert to new uid/gid option parsing helpers

[Eric Sandeen]

On 6/28/24 7:07 AM, Christian Brauner wrote:
> I think you accidently Cced the wrong Miklos. :)
> 
> On Thu, Jun 27, 2024 at 07:33:43PM GMT, Eric Sandeen wrote:
>> Convert to new uid/gid option parsing helpers
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/fuse/inode.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 99e44ea7d875..1ac528bcdb3c 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -740,8 +740,8 @@ static const struct fs_parameter_spec fuse_fs_parameters[] = {
>>  	fsparam_string	("source",		OPT_SOURCE),
>>  	fsparam_u32	("fd",			OPT_FD),
>>  	fsparam_u32oct	("rootmode",		OPT_ROOTMODE),
>> -	fsparam_u32	("user_id",		OPT_USER_ID),
>> -	fsparam_u32	("group_id",		OPT_GROUP_ID),
>> +	fsparam_uid	("user_id",		OPT_USER_ID),
>> +	fsparam_gid	("group_id",		OPT_GROUP_ID),
>>  	fsparam_flag	("default_permissions",	OPT_DEFAULT_PERMISSIONS),
>>  	fsparam_flag	("allow_other",		OPT_ALLOW_OTHER),
>>  	fsparam_u32	("max_read",		OPT_MAX_READ),
>> @@ -799,16 +799,12 @@ static int fuse_parse_param(struct fs_context *fsc, struct fs_parameter *param)
>>  		break;
>>  
>>  	case OPT_USER_ID:
>> -		ctx->user_id = make_kuid(fsc->user_ns, result.uint_32);
>> -		if (!uid_valid(ctx->user_id))
>> -			return invalfc(fsc, "Invalid user_id");
>> +		ctx->user_id = result.uid;
> 
> So fsc->user_ns will record the namespaces at fsopen() time which can be
> different from the namespace used at fsconfig() time. This was done when
> fuse was ported to the new mount api.
> 
> It has the same potential issues that Seth pointed out so I think your
> patch is correct. But I also think we might need the same verification
> that tmpfs is doing to verify that the uid/gid we're using can actually
> be represented in the fsc->user_ns.

Hm yeah, so that's a current bug in fuse, right? (it /would/ be really
nice to be able to spot FS_USERNS_MOUNT in the helper, and do the right
thing in all cases) 

> So maybe there should be a separate patch that converts fuse to rely on
> make_k*id(current_user_ns()) + k*id_has_mapping() and then these patches
> on top?

Ok, I guess the idea is that that patch could land more quickly than this
treewide-ish change (and would be cherry-pickable -stable material), to fix
the bug, and then make this change later. Seems fair.

Thanks,
-Eric

Re: [PATCH 01/14] fs_parse: add uid & gid option option parsing helpers

[Eric Sandeen]

On 6/28/24 4:45 AM, Jan Kara wrote:
> On Thu 27-06-24 19:26:24, Eric Sandeen wrote:
>> Multiple filesystems take uid and gid as options, and the code to
>> create the ID from an integer and validate it is standard boilerplate
>> that can be moved into common helper functions, so do that for
>> consistency and less cut&paste.
>>
>> This also helps avoid the buggy pattern noted by Seth Jenkins at
>> https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
>> because uid/gid parsing will fail before any assignment in most
>> filesystems.
>>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> 
> I like the idea since this seems like a nobrainer but is actually
> surprisingly subtle...
> 
>> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> index a4d6ca0b8971..24727ec34e5a 100644
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
>>  }
>>  EXPORT_SYMBOL(fs_param_is_fd);
>>  
>> +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
>> +		    struct fs_parameter *param, struct fs_parse_result *result)
>> +{
>> +	kuid_t uid;
>> +
>> +	if (fs_param_is_u32(log, p, param, result) != 0)
>> +		return fs_param_bad_value(log, param);
>> +
>> +	uid = make_kuid(current_user_ns(), result->uint_32);
> 
> But here is the problem: Filesystems mountable in user namespaces need to use
> fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()).
> Having helpers that work for some filesystems and are subtly broken for
> others is worse than no helpers... Or am I missing something?

Yeah, I should have pointed that out. tmpfs still does that check after the
initial trivial parsing after this change to use the basic helper:

        case Opt_uid:
                kuid = result.uid;
        
                /*
                 * The requested uid must be representable in the
                 * filesystem's idmapping.
                 */
                if (!kuid_has_mapping(fc->user_ns, kuid))
                        goto bad_value;
        
                ctx->uid = kuid;
                break;

I can see your point about risks of a helper that doesn't cover all cases
though.
 
> And the problem with fc->user_ns is that currently __fs_parse() does not
> get fs_context as an argument... So that will need some larger work.

Yup, this was discussed a little when I sent this idea as an RFC, and the
(brief/small) consensus was that it was worth going this far for now.

Getting fc back into __fs_parse looks rather tricky and Al was not keen
on the idea, for some reason.

Thanks,
-Eric
> 								Honza

Re: [PATCH 02/14] autofs: Convert to new uid/gid option parsing helpers

[Ian Kent]

On 28/6/24 08:27, Eric Sandeen wrote:
> Convert to new uid/gid option parsing helpers
>
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> ---
>   fs/autofs/inode.c | 16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
> index 1f5db6863663..cf792d4de4f1 100644
> --- a/fs/autofs/inode.c
> +++ b/fs/autofs/inode.c
> @@ -126,7 +126,7 @@ enum {
>   const struct fs_parameter_spec autofs_param_specs[] = {
>   	fsparam_flag	("direct",		Opt_direct),
>   	fsparam_fd	("fd",			Opt_fd),
> -	fsparam_u32	("gid",			Opt_gid),
> +	fsparam_gid	("gid",			Opt_gid),
>   	fsparam_flag	("ignore",		Opt_ignore),
>   	fsparam_flag	("indirect",		Opt_indirect),
>   	fsparam_u32	("maxproto",		Opt_maxproto),
> @@ -134,7 +134,7 @@ const struct fs_parameter_spec autofs_param_specs[] = {
>   	fsparam_flag	("offset",		Opt_offset),
>   	fsparam_u32	("pgrp",		Opt_pgrp),
>   	fsparam_flag	("strictexpire",	Opt_strictexpire),
> -	fsparam_u32	("uid",			Opt_uid),
> +	fsparam_uid	("uid",			Opt_uid),
>   	{}
>   };
>   
> @@ -193,8 +193,6 @@ static int autofs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   	struct autofs_fs_context *ctx = fc->fs_private;
>   	struct autofs_sb_info *sbi = fc->s_fs_info;
>   	struct fs_parse_result result;
> -	kuid_t uid;
> -	kgid_t gid;
>   	int opt;
>   
>   	opt = fs_parse(fc, autofs_param_specs, param, &result);
> @@ -205,16 +203,10 @@ static int autofs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   	case Opt_fd:
>   		return autofs_parse_fd(fc, sbi, param, &result);
>   	case Opt_uid:
> -		uid = make_kuid(current_user_ns(), result.uint_32);
> -		if (!uid_valid(uid))
> -			return invalfc(fc, "Invalid uid");
> -		ctx->uid = uid;
> +		ctx->uid = result.uid;
>   		break;
>   	case Opt_gid:
> -		gid = make_kgid(current_user_ns(), result.uint_32);
> -		if (!gid_valid(gid))
> -			return invalfc(fc, "Invalid gid");
> -		ctx->gid = gid;
> +		ctx->gid = result.gid;
>   		break;
>   	case Opt_pgrp:
>   		ctx->pgrp = result.uint_32;


I like the idea and it looks just fine for autofs.

Acked-by: Ian Kent <raven@themaw.net>


Ian

Re: [PATCH 01/14] fs_parse: add uid & gid option option parsing helpers

[Jan Kara]

On Fri 28-06-24 14:23:35, Christian Brauner wrote:
> On Fri, Jun 28, 2024 at 11:45:17AM GMT, Jan Kara wrote:
> > On Thu 27-06-24 19:26:24, Eric Sandeen wrote:
> > > Multiple filesystems take uid and gid as options, and the code to
> > > create the ID from an integer and validate it is standard boilerplate
> > > that can be moved into common helper functions, so do that for
> > > consistency and less cut&paste.
> > > 
> > > This also helps avoid the buggy pattern noted by Seth Jenkins at
> > > https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
> > > because uid/gid parsing will fail before any assignment in most
> > > filesystems.
> > > 
> > > Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> > 
> > I like the idea since this seems like a nobrainer but is actually
> > surprisingly subtle...
> > 
> > > diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > > index a4d6ca0b8971..24727ec34e5a 100644
> > > --- a/fs/fs_parser.c
> > > +++ b/fs/fs_parser.c
> > > @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
> > >  }
> > >  EXPORT_SYMBOL(fs_param_is_fd);
> > >  
> > > +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
> > > +		    struct fs_parameter *param, struct fs_parse_result *result)
> > > +{
> > > +	kuid_t uid;
> > > +
> > > +	if (fs_param_is_u32(log, p, param, result) != 0)
> > > +		return fs_param_bad_value(log, param);
> > > +
> > > +	uid = make_kuid(current_user_ns(), result->uint_32);
> > 
> > But here is the problem: Filesystems mountable in user namespaces need to use
> > fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()).
> > Having helpers that work for some filesystems and are subtly broken for
> > others is worse than no helpers... Or am I missing something?
> > 
> > And the problem with fc->user_ns is that currently __fs_parse() does not
> > get fs_context as an argument... So that will need some larger work.
> 
> Not really. If someone does an fsopen() in a namespace but the process
> that actually sets mount options is in another namespace then it's
> completely intransparent what uid/gid this will resolve to if it's
> resovled according to fsopen().
> 
> It's also a bit strange if someone ends up handing off a tmpfs fscontext
> that was created in the initial namespace to some random namespace and
> they now can set uid/gid options that aren't mapped according to their
> namespace but instead are 1:1 resolved according to the intial
> namespace. So this would hinder delegation.
> 
> The expectation is that uid/gid options are resolved in the caller's
> namespace and that shouldn't be any different for fscontexts for
> namespace mountable filesystems. The crucial point is to ensure that the
> resulting kuid/kgid can be resolved in the namespace the filesystem is
> mounted in at the end. That's what was lacking in e.g., tmpfs in commit
> 0200679fc795 ("tmpfs: verify {g,u}id mount options correctly")
> 
> The fuse conversion is the only inconsistency in that regard.

OK, thanks for explanation!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: (subset) [PATCH 0/14] New uid & gid mount option parsing helpers

[Christian Brauner]

On Thu, 27 Jun 2024 19:24:59 -0500, Eric Sandeen wrote:
> Multiple filesystems take uid and gid as options, and the code to
> create the ID from an integer and validate it is standard boilerplate
> that can be moved into common helper functions, so do that for
> consistency and less cut&paste.
> 
> This also helps avoid the buggy pattern noted by Seth Jenkins at
> https://lore.kernel.org/lkml/CALxfFW4BXhEwxR0Q5LSkg-8Vb4r2MONKCcUCVioehXQKr35eHg@mail.gmail.com/
> because uid/gid parsing will fail before any assignment in most
> filesystems.
> 
> [...]

I've snatched everything but the fuse change as we should do that one in
two steps.

---

Applied to the vfs.mount.api branch of the vfs/vfs.git tree.
Patches in the vfs.mount.api branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.mount.api

[01/14] fs_parse: add uid & gid option option parsing helpers
        https://git.kernel.org/vfs/vfs/c/9f111059e725
[02/14] autofs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/748cddf13de5
[03/14] debugfs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/49abee5991e1
[04/14] efivarfs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/dcffad38c767
[05/14] exfat: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/ffe1b94d7464
[06/14] ext4: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/6b5732b5ca4f
[08/14] hugetlbfs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/eefc13247722
[09/14] isofs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/6a265845db28
[10/14] ntfs3: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/c449cb5d1bce
[11/14] tmpfs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/2ec07010b6a9
[12/14] smb: client: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/3229e3a5a374
[13/14] tracefs: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/b548291690d1
[14/14] vboxsf: Convert to new uid/gid option parsing helpers
        https://git.kernel.org/vfs/vfs/c/da99d45bd551