diff mbox series

[v3,1/2] fs: add the ability for statmount() to report the fs_subtype

Message ID 20241107-statmount-v3-1-da5b9744c121@kernel.org (mailing list archive)
State New
Headers show
Series fs: allow statmount to fetch the subtype and devname | expand

Commit Message

Jeff Layton Nov. 7, 2024, 9 p.m. UTC 
/proc/self/mountinfo prints out the sb->s_subtype after the type. This
is particularly useful for disambiguating FUSE mounts (at least when the
userland driver bothers to set it).

Add STATMOUNT_FS_SUBTYPE and claim one of the __spare2 fields to point
to the offset into the str[] array.

Handle the case where there is no subtype by not setting
STATMOUNT_FS_SUBTYPE in the returned mask. Check whether the function
emitted anything and just return immediately if not.

Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/namespace.c             | 34 ++++++++++++++++++++++++++++------
 include/uapi/linux/mount.h |  5 ++++-
 2 files changed, 32 insertions(+), 7 deletions(-)

Comments

Miklos Szeredi Nov. 11, 2024, 10:49 a.m. UTC | #1 
On Thu, 7 Nov 2024 at 22:00, Jeff Layton <jlayton@kernel.org> wrote:

> +       /*
> +        * If nothing was emitted, return to avoid setting the flag
> +        * and terminating the buffer.
> +        */
> +       if (seq->count == start)
> +               return ret;

First of all, I don't think it's okay to subtly change behavior of
other string attributes in this patch.   If that is what we want, it
should be separated into a separate prep or followup patch.

Clearing the returned mask if there's no subtype does sound like the
right thing to do.  But it makes it impossible to detect whether the
running kernel supports returning subtype or not.  Missing the
STATMOUNT_FS_SUBTYPE in statmount.mask may mean two different things:

 - kernel supports returning subtype and filesystem does not have a subtype

 - kernel does not support returning a subtype and the filesystem may
or may not have a subtype

I think we can live with  that, but it would be really good if there
was a universal way to detect whether a particular feature is
supported on the running kernel or not, and not have to rely on
syscall specific ways.

Thanks,
Miklos
Jeff Layton Nov. 11, 2024, 11:28 a.m. UTC | #2 
On Mon, 2024-11-11 at 11:49 +0100, Miklos Szeredi wrote:
> On Thu, 7 Nov 2024 at 22:00, Jeff Layton <jlayton@kernel.org> wrote:
> 
> > +       /*
> > +        * If nothing was emitted, return to avoid setting the flag
> > +        * and terminating the buffer.
> > +        */
> > +       if (seq->count == start)
> > +               return ret;
> 
> First of all, I don't think it's okay to subtly change behavior of
> other string attributes in this patch.   If that is what we want, it
> should be separated into a separate prep or followup patch.
> 

As far as I can tell, the existing cases in statmount_string() either
always emit a string or an error code. If a string isn't emitted, then
the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
think this will result in any change in behavior for the existing code.


> Clearing the returned mask if there's no subtype does sound like the
> right thing to do.  But it makes it impossible to detect whether the
> running kernel supports returning subtype or not.  Missing the
> STATMOUNT_FS_SUBTYPE in statmount.mask may mean two different things:
> 
>  - kernel supports returning subtype and filesystem does not have a subtype
> 
>  - kernel does not support returning a subtype and the filesystem may
> or may not have a subtype
> 
> I think we can live with  that, but it would be really good if there
> was a universal way to detect whether a particular feature is
> supported on the running kernel or not, and not have to rely on
> syscall specific ways.

The idea for statmount() was to add a statx() like interface. This is
exactly how statx() works, so I don't think it ought to be any sort of
surprise to anyone.

That said, if we did want to add a way to detect what flags are
actually supported, we could just add a new STATMOUNT_SUPPORTED_FLAGS
flag and add a field that holds a returned mask with all of the bits
set that the kernel supports.
Miklos Szeredi Nov. 11, 2024, 1:01 p.m. UTC | #3 
On Mon, 11 Nov 2024 at 12:28, Jeff Layton <jlayton@kernel.org> wrote:

> As far as I can tell, the existing cases in statmount_string() either
> always emit a string or an error code. If a string isn't emitted, then
> the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
> think this will result in any change in behavior for the existing code.

Both mnt_point and mnt_opts can be empty.

> The idea for statmount() was to add a statx() like interface. This is
> exactly how statx() works, so I don't think it ought to be any sort of
> surprise to anyone.

Maybe, but silently changing the interface is not okay.  At least make
it a separate patch.

> That said, if we did want to add a way to detect what flags are
> actually supported, we could just add a new STATMOUNT_SUPPORTED_FLAGS
> flag and add a field that holds a returned mask with all of the bits
> set that the kernel supports.

Yeah, that makes sense.

Thanks,
Miklos
Jeff Layton Nov. 11, 2024, 1:28 p.m. UTC | #4 
On Mon, 2024-11-11 at 14:01 +0100, Miklos Szeredi wrote:
> On Mon, 11 Nov 2024 at 12:28, Jeff Layton <jlayton@kernel.org> wrote:
> 
> > As far as I can tell, the existing cases in statmount_string() either
> > always emit a string or an error code. If a string isn't emitted, then
> > the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
> > think this will result in any change in behavior for the existing code.
> 
> Both mnt_point and mnt_opts can be empty.
> 

Ok, so currently if they are, the flag gets set and there is no
string?  If so, then you're correct and this is a behavior change. The
question is -- is it a desirable one? The interface is new enough that
I think we have the luxury of changing this now (and establishing a
future standard).

Personally, I think that's how it ought to work. When there is no
string present, we ought not set the flag in the return mask. Does
anyone prefer it the other way?

> > The idea for statmount() was to add a statx() like interface. This is
> > exactly how statx() works, so I don't think it ought to be any sort of
> > surprise to anyone.
> 
> Maybe, but silently changing the interface is not okay.  At least make
> it a separate patch.
> 

Ok, Christian has already taken this in, but I can respin it and 
split that bit out into a separate patch. Or, I could just revise the
changelog to note the behavior change.

Either way, we'll need to decide about how empty fields should be
handled first.

> > That said, if we did want to add a way to detect what flags are
> > actually supported, we could just add a new STATMOUNT_SUPPORTED_FLAGS
> > flag and add a field that holds a returned mask with all of the bits
> > set that the kernel supports.
> 
> Yeah, that makes sense.
>
Christian Brauner Nov. 11, 2024, 1:30 p.m. UTC | #5 
On Mon, Nov 11, 2024 at 08:28:07AM -0500, Jeff Layton wrote:
> On Mon, 2024-11-11 at 14:01 +0100, Miklos Szeredi wrote:
> > On Mon, 11 Nov 2024 at 12:28, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > > As far as I can tell, the existing cases in statmount_string() either
> > > always emit a string or an error code. If a string isn't emitted, then
> > > the two EOVERFLOW cases and the EAGAIN case can't happen, so I don't
> > > think this will result in any change in behavior for the existing code.
> > 
> > Both mnt_point and mnt_opts can be empty.
> > 
> 
> Ok, so currently if they are, the flag gets set and there is no
> string?  If so, then you're correct and this is a behavior change. The
> question is -- is it a desirable one? The interface is new enough that
> I think we have the luxury of changing this now (and establishing a
> future standard).
> 
> Personally, I think that's how it ought to work. When there is no
> string present, we ought not set the flag in the return mask. Does
> anyone prefer it the other way?

It's a change we can certainly do. I see no reason not to try it.
diff mbox series

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index ba77ce1c6788dfe461814b5826fcbb3aab68fad4..fc4f81891d544305caf863904c0a6e16562fab49 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5006,6 +5006,14 @@  static int statmount_fs_type(struct kstatmount *s, struct seq_file *seq)
 	return 0;
 }
 
+static void statmount_fs_subtype(struct kstatmount *s, struct seq_file *seq)
+{
+	struct super_block *sb = s->mnt->mnt_sb;
+
+	if (sb->s_subtype)
+		seq_puts(seq, sb->s_subtype);
+}
+
 static void statmount_mnt_ns_id(struct kstatmount *s, struct mnt_namespace *ns)
 {
 	s->sm.mask |= STATMOUNT_MNT_NS_ID;
@@ -5042,33 +5050,44 @@  static int statmount_mnt_opts(struct kstatmount *s, struct seq_file *seq)
 
 static int statmount_string(struct kstatmount *s, u64 flag)
 {
-	int ret;
+	int ret = 0;
 	size_t kbufsize;
 	struct seq_file *seq = &s->seq;
 	struct statmount *sm = &s->sm;
+	u32 start = seq->count;
 
 	switch (flag) {
 	case STATMOUNT_FS_TYPE:
-		sm->fs_type = seq->count;
+		sm->fs_type = start;
 		ret = statmount_fs_type(s, seq);
 		break;
 	case STATMOUNT_MNT_ROOT:
-		sm->mnt_root = seq->count;
+		sm->mnt_root = start;
 		ret = statmount_mnt_root(s, seq);
 		break;
 	case STATMOUNT_MNT_POINT:
-		sm->mnt_point = seq->count;
+		sm->mnt_point = start;
 		ret = statmount_mnt_point(s, seq);
 		break;
 	case STATMOUNT_MNT_OPTS:
-		sm->mnt_opts = seq->count;
+		sm->mnt_opts = start;
 		ret = statmount_mnt_opts(s, seq);
 		break;
+	case STATMOUNT_FS_SUBTYPE:
+		sm->fs_subtype = start;
+		statmount_fs_subtype(s, seq);
+		break;
 	default:
 		WARN_ON_ONCE(true);
 		return -EINVAL;
 	}
 
+	/*
+	 * If nothing was emitted, return to avoid setting the flag
+	 * and terminating the buffer.
+	 */
+	if (seq->count == start)
+		return ret;
 	if (unlikely(check_add_overflow(sizeof(*sm), seq->count, &kbufsize)))
 		return -EOVERFLOW;
 	if (kbufsize >= s->bufsize)
@@ -5203,6 +5222,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_FS_SUBTYPE)
+		err = statmount_string(s, STATMOUNT_FS_SUBTYPE);
+
 	if (!err && s->mask & STATMOUNT_MNT_NS_ID)
 		statmount_mnt_ns_id(s, ns);
 
@@ -5224,7 +5246,7 @@  static inline bool retry_statmount(const long ret, size_t *seq_size)
 }
 
 #define STATMOUNT_STRING_REQ (STATMOUNT_MNT_ROOT | STATMOUNT_MNT_POINT | \
-			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS)
+			      STATMOUNT_FS_TYPE | STATMOUNT_MNT_OPTS | STATMOUNT_FS_SUBTYPE)
 
 static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
 			      struct statmount __user *buf, size_t bufsize,
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 225bc366ffcbf0319929e2f55f1fbea88e4d7b81..2e939dddf9cbabe574dafdb6cff9ad4cf9298a74 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -173,7 +173,9 @@  struct statmount {
 	__u32 mnt_root;		/* [str] Root of mount relative to root of fs */
 	__u32 mnt_point;	/* [str] Mountpoint relative to current root */
 	__u64 mnt_ns_id;	/* ID of the mount namespace */
-	__u64 __spare2[49];
+	__u32 fs_subtype;	/* [str] Subtype of fs_type (if any) */
+	__u32 __spare1[1];
+	__u64 __spare2[48];
 	char str[];		/* Variable size part containing strings */
 };
 
@@ -207,6 +209,7 @@  struct mnt_id_req {
 #define STATMOUNT_FS_TYPE		0x00000020U	/* Want/got fs_type */
 #define STATMOUNT_MNT_NS_ID		0x00000040U	/* Want/got mnt_ns_id */
 #define STATMOUNT_MNT_OPTS		0x00000080U	/* Want/got mnt_opts */
+#define STATMOUNT_FS_SUBTYPE		0x00000100U	/* Want/got fs_subtype */
 
 /*
  * Special @mnt_id values that can be passed to listmount