[PATCH] statmount: add flag to retrieve unescaped options

[PATCH] statmount: add flag to retrieve unescaped options

[Miklos Szeredi]

Filesystem options can be retrieved with STATMOUNT_MNT_OPTS, which
returns a string of comma separated options, where some characters are
escaped using the \OOO notation.

Add a new flag, STATMOUNT_OPT_ARRAY, which instead returns the raw
option values separated with '\0' charaters.

Since escaped charaters are rare, this inteface is preferable for
non-libmount users which likley don't want to deal with option
de-escaping.

Example code:

	if (st->mask & STATMOUNT_OPT_ARRAY) {
		const char *opt = st->str + st->opt_array;

		for (unsigned int i = 0; i < st->opt_num; i++) {
			printf("opt_array[%i]: <%s>\n", i, opt);
			opt += strlen(opt) + 1;
		}
	}

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c             | 42 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/mount.h |  7 +++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 9a4ab1bc8b94..a16f75011610 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5074,6 +5074,41 @@ static int statmount_mnt_opts(struct kstatmount *s, struct seq_file *seq)
 	return 0;
 }
 
+static int statmount_opt_array(struct kstatmount *s, struct seq_file *seq)
+{
+	struct vfsmount *mnt = s->mnt;
+	struct super_block *sb = mnt->mnt_sb;
+	size_t start = seq->count;
+	u32 count = 0;
+	char *p, *end, *next, *u = seq->buf + start;
+	int err;
+
+       if (!sb->s_op->show_options)
+               return 0;
+
+       err = sb->s_op->show_options(seq, mnt->mnt_root);
+       if (err)
+	       return err;
+
+       if (unlikely(seq_has_overflowed(seq)))
+	       return -EAGAIN;
+
+       end = seq->buf + seq->count;
+       *end = '\0';
+       for (p = u + 1; p < end; p = next + 1) {
+               next = strchrnul(p, ',');
+               *next = '\0';
+               u += string_unescape(p, u, 0, UNESCAPE_OCTAL) + 1;
+	       count++;
+	       if (!count)
+		       return -EOVERFLOW;
+       }
+       seq->count = u - 1 - seq->buf;
+       s->sm.opt_num = count;
+
+       return 0;
+}
+
 static int statmount_string(struct kstatmount *s, u64 flag)
 {
 	int ret = 0;
@@ -5099,6 +5134,10 @@ static int statmount_string(struct kstatmount *s, u64 flag)
 		sm->mnt_opts = start;
 		ret = statmount_mnt_opts(s, seq);
 		break;
+	case STATMOUNT_OPT_ARRAY:
+		sm->opt_array = start;
+		ret = statmount_opt_array(s, seq);
+		break;
 	case STATMOUNT_FS_SUBTYPE:
 		sm->fs_subtype = start;
 		statmount_fs_subtype(s, seq);
@@ -5252,6 +5291,9 @@ static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
 	if (!err && s->mask & STATMOUNT_MNT_OPTS)
 		err = statmount_string(s, STATMOUNT_MNT_OPTS);
 
+	if (!err && s->mask & STATMOUNT_OPT_ARRAY)
+		err = statmount_string(s, STATMOUNT_OPT_ARRAY);
+
 	if (!err && s->mask & STATMOUNT_FS_SUBTYPE)
 		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
 
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 2b49e9131d77..c0fda4604187 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -154,7 +154,7 @@ struct mount_attr {
  */
 struct statmount {
 	__u32 size;		/* Total size, including strings */
-	__u32 mnt_opts;		/* [str] Mount options of the mount */
+	__u32 mnt_opts;		/* [str] Options (comma separated, escaped) */
 	__u64 mask;		/* What results were written */
 	__u32 sb_dev_major;	/* Device ID */
 	__u32 sb_dev_minor;
@@ -175,7 +175,9 @@ struct statmount {
 	__u64 mnt_ns_id;	/* ID of the mount namespace */
 	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
 	__u32 sb_source;	/* [str] Source string of the mount */
-	__u64 __spare2[48];
+	__u32 opt_num;		/* Number of fs options */
+	__u32 opt_array;	/* [str] Array of nul terminated fs options */
+	__u64 __spare2[47];
 	char str[];		/* Variable size part containing strings */
 };
 
@@ -211,6 +213,7 @@ struct mnt_id_req {
 #define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
 #define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
 #define STATMOUNT_SB_SOURCE		0x00000200U	/* Want/got sb_source */
+#define STATMOUNT_OPT_ARRAY		0x00000400U	/* Want/got opt_... */
 
 /*
  * Special @mnt_id values that can be passed to listmount
-- 
2.47.0

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Jeff Layton]

On Tue, 2024-11-12 at 11:10 +0100, Miklos Szeredi wrote:
> Filesystem options can be retrieved with STATMOUNT_MNT_OPTS, which
> returns a string of comma separated options, where some characters are
> escaped using the \OOO notation.
> 
> Add a new flag, STATMOUNT_OPT_ARRAY, which instead returns the raw
> option values separated with '\0' charaters.
> 
> Since escaped charaters are rare, this inteface is preferable for
> non-libmount users which likley don't want to deal with option
> de-escaping.
> 
> Example code:
> 
> 	if (st->mask & STATMOUNT_OPT_ARRAY) {
> 		const char *opt = st->str + st->opt_array;
> 
> 		for (unsigned int i = 0; i < st->opt_num; i++) {
> 			printf("opt_array[%i]: <%s>\n", i, opt);
> 			opt += strlen(opt) + 1;
> 		}
> 	}
> 

If the options are separated by NULs, how does userland know where to
stop?

At some point we will probably end up adding a new string value that
would go after the opt array, and userland will need some way to
clearly tell where that new string begins and the NUL-terminated
options array ends.

> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/namespace.c             | 42 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/mount.h |  7 +++++--
>  2 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 9a4ab1bc8b94..a16f75011610 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -5074,6 +5074,41 @@ static int statmount_mnt_opts(struct kstatmount *s, struct seq_file *seq)
>  	return 0;
>  }
>  
> +static int statmount_opt_array(struct kstatmount *s, struct seq_file *seq)
> +{
> +	struct vfsmount *mnt = s->mnt;
> +	struct super_block *sb = mnt->mnt_sb;
> +	size_t start = seq->count;
> +	u32 count = 0;
> +	char *p, *end, *next, *u = seq->buf + start;
> +	int err;
> +
> +       if (!sb->s_op->show_options)
> +               return 0;
> +
> +       err = sb->s_op->show_options(seq, mnt->mnt_root);
> +       if (err)
> +	       return err;
> +
> +       if (unlikely(seq_has_overflowed(seq)))
> +	       return -EAGAIN;
> +
> +       end = seq->buf + seq->count;
> +       *end = '\0';
> +       for (p = u + 1; p < end; p = next + 1) {
> +               next = strchrnul(p, ',');
> +               *next = '\0';
> +               u += string_unescape(p, u, 0, UNESCAPE_OCTAL) + 1;
> +	       count++;
> +	       if (!count)
> +		       return -EOVERFLOW;
> +       }
> +       seq->count = u - 1 - seq->buf;
> +       s->sm.opt_num = count;
> +
> +       return 0;
> +}
> +
>  static int statmount_string(struct kstatmount *s, u64 flag)
>  {
>  	int ret = 0;
> @@ -5099,6 +5134,10 @@ static int statmount_string(struct kstatmount *s, u64 flag)
>  		sm->mnt_opts = start;
>  		ret = statmount_mnt_opts(s, seq);
>  		break;
> +	case STATMOUNT_OPT_ARRAY:
> +		sm->opt_array = start;
> +		ret = statmount_opt_array(s, seq);
> +		break;
>  	case STATMOUNT_FS_SUBTYPE:
>  		sm->fs_subtype = start;
>  		statmount_fs_subtype(s, seq);
> @@ -5252,6 +5291,9 @@ static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
>  	if (!err && s->mask & STATMOUNT_MNT_OPTS)
>  		err = statmount_string(s, STATMOUNT_MNT_OPTS);
>  
> +	if (!err && s->mask & STATMOUNT_OPT_ARRAY)
> +		err = statmount_string(s, STATMOUNT_OPT_ARRAY);
> +
>  	if (!err && s->mask & STATMOUNT_FS_SUBTYPE)
>  		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
>  
> diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
> index 2b49e9131d77..c0fda4604187 100644
> --- a/include/uapi/linux/mount.h
> +++ b/include/uapi/linux/mount.h
> @@ -154,7 +154,7 @@ struct mount_attr {
>   */
>  struct statmount {
>  	__u32 size;		/* Total size, including strings */
> -	__u32 mnt_opts;		/* [str] Mount options of the mount */
> +	__u32 mnt_opts;		/* [str] Options (comma separated, escaped) */
>  	__u64 mask;		/* What results were written */
>  	__u32 sb_dev_major;	/* Device ID */
>  	__u32 sb_dev_minor;
> @@ -175,7 +175,9 @@ struct statmount {
>  	__u64 mnt_ns_id;	/* ID of the mount namespace */
>  	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
>  	__u32 sb_source;	/* [str] Source string of the mount */
> -	__u64 __spare2[48];
> +	__u32 opt_num;		/* Number of fs options */
> +	__u32 opt_array;	/* [str] Array of nul terminated fs options */
> +	__u64 __spare2[47];
>  	char str[];		/* Variable size part containing strings */
>  };
>  
> @@ -211,6 +213,7 @@ struct mnt_id_req {
>  #define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
>  #define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
>  #define STATMOUNT_SB_SOURCE		0x00000200U	/* Want/got sb_source */
> +#define STATMOUNT_OPT_ARRAY		0x00000400U	/* Want/got opt_... */
>  
>  /*
>   * Special @mnt_id values that can be passed to listmount

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Miklos Szeredi]

On Tue, 12 Nov 2024 at 13:20, Jeff Layton <jlayton@kernel.org> wrote:

> If the options are separated by NULs, how does userland know where to
> stop?

The number of options is returned in st->opt_num.  I find that clearer
than returning the end offset.

Thanks,
Miklos

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Jeff Layton]

On Tue, 2024-11-12 at 07:20 -0500, Jeff Layton wrote:
> On Tue, 2024-11-12 at 11:10 +0100, Miklos Szeredi wrote:
> > Filesystem options can be retrieved with STATMOUNT_MNT_OPTS, which
> > returns a string of comma separated options, where some characters are
> > escaped using the \OOO notation.
> > 
> > Add a new flag, STATMOUNT_OPT_ARRAY, which instead returns the raw
> > option values separated with '\0' charaters.
> > 
> > Since escaped charaters are rare, this inteface is preferable for
> > non-libmount users which likley don't want to deal with option
> > de-escaping.
> > 
> > Example code:
> > 
> > 	if (st->mask & STATMOUNT_OPT_ARRAY) {
> > 		const char *opt = st->str + st->opt_array;
> > 
> > 		for (unsigned int i = 0; i < st->opt_num; i++) {
> > 			printf("opt_array[%i]: <%s>\n", i, opt);
> > 			opt += strlen(opt) + 1;
> > 		}
> > 	}
> > 
> 
> If the options are separated by NULs, how does userland know where to
> stop?
> 
> At some point we will probably end up adding a new string value that
> would go after the opt array, and userland will need some way to
> clearly tell where that new string begins and the NUL-terminated
> options array ends.
> 

Ok, that should work.

> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/namespace.c             | 42 ++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/mount.h |  7 +++++--
> >  2 files changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 9a4ab1bc8b94..a16f75011610 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -5074,6 +5074,41 @@ static int statmount_mnt_opts(struct kstatmount *s, struct seq_file *seq)
> >  	return 0;
> >  }
> >  
> > +static int statmount_opt_array(struct kstatmount *s, struct seq_file *seq)
> > +{
> > +	struct vfsmount *mnt = s->mnt;
> > +	struct super_block *sb = mnt->mnt_sb;
> > +	size_t start = seq->count;
> > +	u32 count = 0;
> > +	char *p, *end, *next, *u = seq->buf + start;
> > +	int err;
> > +
> > +       if (!sb->s_op->show_options)
> > +               return 0;
> > +
> > +       err = sb->s_op->show_options(seq, mnt->mnt_root);
> > +       if (err)
> > +	       return err;
> > +
> > +       if (unlikely(seq_has_overflowed(seq)))
> > +	       return -EAGAIN;
> > +
> > +       end = seq->buf + seq->count;
> > +       *end = '\0';
> > +       for (p = u + 1; p < end; p = next + 1) {
> > +               next = strchrnul(p, ',');
> > +               *next = '\0';
> > +               u += string_unescape(p, u, 0, UNESCAPE_OCTAL) + 1;
> > +	       count++;
> > +	       if (!count)
> > +		       return -EOVERFLOW;
> > +       }
> > +       seq->count = u - 1 - seq->buf;
> > +       s->sm.opt_num = count;
> > +
> > +       return 0;
> > +}
> > +
> >  static int statmount_string(struct kstatmount *s, u64 flag)
> >  {
> >  	int ret = 0;
> > @@ -5099,6 +5134,10 @@ static int statmount_string(struct kstatmount *s, u64 flag)
> >  		sm->mnt_opts = start;
> >  		ret = statmount_mnt_opts(s, seq);
> >  		break;
> > +	case STATMOUNT_OPT_ARRAY:
> > +		sm->opt_array = start;
> > +		ret = statmount_opt_array(s, seq);
> > +		break;
> >  	case STATMOUNT_FS_SUBTYPE:
> >  		sm->fs_subtype = start;
> >  		statmount_fs_subtype(s, seq);
> > @@ -5252,6 +5291,9 @@ static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
> >  	if (!err && s->mask & STATMOUNT_MNT_OPTS)
> >  		err = statmount_string(s, STATMOUNT_MNT_OPTS);
> >  
> > +	if (!err && s->mask & STATMOUNT_OPT_ARRAY)
> > +		err = statmount_string(s, STATMOUNT_OPT_ARRAY);
> > +
> >  	if (!err && s->mask & STATMOUNT_FS_SUBTYPE)
> >  		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
> >  
> > diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
> > index 2b49e9131d77..c0fda4604187 100644
> > --- a/include/uapi/linux/mount.h
> > +++ b/include/uapi/linux/mount.h
> > @@ -154,7 +154,7 @@ struct mount_attr {
> >   */
> >  struct statmount {
> >  	__u32 size;		/* Total size, including strings */
> > -	__u32 mnt_opts;		/* [str] Mount options of the mount */
> > +	__u32 mnt_opts;		/* [str] Options (comma separated, escaped) */
> >  	__u64 mask;		/* What results were written */
> >  	__u32 sb_dev_major;	/* Device ID */
> >  	__u32 sb_dev_minor;
> > @@ -175,7 +175,9 @@ struct statmount {
> >  	__u64 mnt_ns_id;	/* ID of the mount namespace */
> >  	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
> >  	__u32 sb_source;	/* [str] Source string of the mount */
> > -	__u64 __spare2[48];
> > +	__u32 opt_num;		/* Number of fs options */

Since there are 2 ways to get mount options, maybe make this more
clear: "Number of fs options in opt_array".

> > +	__u32 opt_array;	/* [str] Array of nul terminated fs options */
> > +	__u64 __spare2[47];
> >  	char str[];		/* Variable size part containing strings */
> >  };
> >  
> > @@ -211,6 +213,7 @@ struct mnt_id_req {
> >  #define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
> >  #define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
> >  #define STATMOUNT_SB_SOURCE		0x00000200U	/* Want/got sb_source */
> > +#define STATMOUNT_OPT_ARRAY		0x00000400U	/* Want/got opt_... */
> >  
> >  /*
> >   * Special @mnt_id values that can be passed to listmount
> 

Acked-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Christian Brauner]

On Tue, Nov 12, 2024 at 11:10:04AM +0100, Miklos Szeredi wrote:
> Filesystem options can be retrieved with STATMOUNT_MNT_OPTS, which
> returns a string of comma separated options, where some characters are
> escaped using the \OOO notation.
> 
> Add a new flag, STATMOUNT_OPT_ARRAY, which instead returns the raw
> option values separated with '\0' charaters.
> 
> Since escaped charaters are rare, this inteface is preferable for
> non-libmount users which likley don't want to deal with option
> de-escaping.
> 
> Example code:
> 
> 	if (st->mask & STATMOUNT_OPT_ARRAY) {
> 		const char *opt = st->str + st->opt_array;
> 
> 		for (unsigned int i = 0; i < st->opt_num; i++) {
> 			printf("opt_array[%i]: <%s>\n", i, opt);
> 			opt += strlen(opt) + 1;
> 		}
> 	}
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

I'm likely going to snatch this for v6.13 still. I just need to reflow
the code as the formatting is broken and maybe rename a few variables
and need to wrap my head around the parsing. I'm testing this now.

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Christian Brauner]

On Wed, Nov 13, 2024 at 01:27:08PM +0100, Christian Brauner wrote:
> On Tue, Nov 12, 2024 at 11:10:04AM +0100, Miklos Szeredi wrote:
> > Filesystem options can be retrieved with STATMOUNT_MNT_OPTS, which
> > returns a string of comma separated options, where some characters are
> > escaped using the \OOO notation.
> > 
> > Add a new flag, STATMOUNT_OPT_ARRAY, which instead returns the raw
> > option values separated with '\0' charaters.
> > 
> > Since escaped charaters are rare, this inteface is preferable for
> > non-libmount users which likley don't want to deal with option
> > de-escaping.
> > 
> > Example code:
> > 
> > 	if (st->mask & STATMOUNT_OPT_ARRAY) {
> > 		const char *opt = st->str + st->opt_array;
> > 
> > 		for (unsigned int i = 0; i < st->opt_num; i++) {
> > 			printf("opt_array[%i]: <%s>\n", i, opt);
> > 			opt += strlen(opt) + 1;
> > 		}
> > 	}
> > 
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> 
> I'm likely going to snatch this for v6.13 still. I just need to reflow
> the code as the formatting is broken and maybe rename a few variables
> and need to wrap my head around the parsing. I'm testing this now.

Please take a look at the top of #vfs.misc and tell me whether this is
ok with you.

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Miklos Szeredi]

On Wed, 13 Nov 2024 at 17:31, Christian Brauner <brauner@kernel.org> wrote:

> Please take a look at the top of #vfs.misc and tell me whether this is
> ok with you.

One more thing I'd do is:

-       if (seq->count == start)
+       if (seq->count <= start + 1)

This would handle the (broken) case of just a single comma in the
options.  So it's not a bug fix per-se, but would make it clear that
the loop will run at least once, and so the seq->count calculation at
the end won't be skewed.

The buf_start calculation could also be moved further down before the
loop, where it's actually used.

I don't find the variable naming much better, how about taking the
naming from string_unescape:

opt_start -> src - pointer to escaped string
buf_start -> dst - pointer to de-escaped string

The others make sense.

Thanks,
Miklos

Re: [PATCH] statmount: add flag to retrieve unescaped options

[Christian Brauner]

On Thu, Nov 14, 2024 at 03:53:44PM +0100, Miklos Szeredi wrote:
> On Wed, 13 Nov 2024 at 17:31, Christian Brauner <brauner@kernel.org> wrote:
> 
> > Please take a look at the top of #vfs.misc and tell me whether this is
> > ok with you.
> 
> One more thing I'd do is:
> 
> -       if (seq->count == start)
> +       if (seq->count <= start + 1)
> 
> This would handle the (broken) case of just a single comma in the
> options.  So it's not a bug fix per-se, but would make it clear that
> the loop will run at least once, and so the seq->count calculation at
> the end won't be skewed.
> 
> The buf_start calculation could also be moved further down before the
> loop, where it's actually used.
> 
> I don't find the variable naming much better, how about taking the
> naming from string_unescape:
> 
> opt_start -> src - pointer to escaped string
> buf_start -> dst - pointer to de-escaped string
> 
> The others make sense.

Fine by me. I'm about to send pull-requests so I would suggest you send
a cleanup that I can include post-rc1 so we don't have to rebase right
now.