[PATCH 0/6] filesystem visibility ioctls

[PATCH 0/6] filesystem visibility ioctls

[Kent Overstreet]

Hi all,

this patchset adds a few new ioctls to standardize a few interfaces we
want
 - get/set UUID
 - get sysfs path

The get/set UUID ioctls are lifted versions of the ext4 ioctls with one
difference, killing the flexible array member - we'll never have UUIDs
more than 16 bytes, and getting rid of the flexible array member makes
them easier to use.

FS_IOC_GETSYSFSNAME is new, but it addresses something that we've been
doing in fs specific code for awhile - "given a path on a mounted
filesystem, tell me where it lives in sysfs".

Cheers,
Kent

Kent Overstreet (6):
  fs: super_block->s_uuid_len
  fs: FS_IOC_GETUUID
  fat: Hook up sb->s_uuid
  fs: FS_IOC_GETSYSFSNAME
  xfs: add support for FS_IOC_GETSYSFSNAME
  bcachefs: add support for FS_IOC_GETSYSFSNAME

 fs/bcachefs/fs.c        |  1 +
 fs/fat/inode.c          |  4 ++++
 fs/ioctl.c              | 33 +++++++++++++++++++++++++++++++++
 fs/super.c              |  1 +
 fs/xfs/xfs_mount.c      |  2 ++
 include/linux/fs.h      |  2 ++
 include/uapi/linux/fs.h | 21 +++++++++++++++++++++
 7 files changed, 64 insertions(+)

-- 
2.43.0

[PATCH 1/6] fs: super_block->s_uuid_len

[Kent Overstreet]

Some weird old filesytems have UUID-like things that we wish to expose
as UUIDs, but are smaller; add a length field so that the new
FS_IOC_(GET|SET)UUID ioctls can handle them in generic code.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/super.c         | 1 +
 include/linux/fs.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index d35e85295489..ed688d2a58a7 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -375,6 +375,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	s->s_time_gran = 1000000000;
 	s->s_time_min = TIME64_MIN;
 	s->s_time_max = TIME64_MAX;
+	s->s_uuid_len = sizeof(s->s_uuid);
 
 	s->s_shrink = shrinker_alloc(SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE,
 				     "sb-%s", type->name);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ed5966a70495..ff41ea6c3a9c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1257,6 +1257,7 @@ struct super_block {
 
 	char			s_id[32];	/* Informational name */
 	uuid_t			s_uuid;		/* UUID */
+	u8			s_uuid_len;	/* Default 16, possibly smaller for weird filesystems */
 
 	unsigned int		s_max_links;
 
-- 
2.43.0

[PATCH 2/6] fs: FS_IOC_GETUUID

[Kent Overstreet]

Add a new generic ioctls for querying the filesystem UUID.

These are lifted versions of the ext4 ioctls, with one change: we're not
using a flexible array member, because UUIDs will never be more than 16
bytes.

This patch adds a generic implementation of FS_IOC_GETFSUUID, which
reads from super_block->s_uuid; FS_IOC_SETFSUUID is left for individual
filesystems to implement.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.or
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/ioctl.c              | 16 ++++++++++++++++
 include/uapi/linux/fs.h | 16 ++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..858801060408 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
 	return err;
 }
 
+static int ioctl_getfsuuid(struct file *file, void __user *argp)
+{
+	struct super_block *sb = file_inode(file)->i_sb;
+
+	if (WARN_ON(sb->s_uuid_len > sizeof(sb->s_uuid)))
+		sb->s_uuid_len = sizeof(sb->s_uuid);
+
+	struct fsuuid2 u = { .fsu_len = sb->s_uuid_len, };
+	memcpy(&u.fsu_uuid[0], &sb->s_uuid, sb->s_uuid_len);
+
+	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
+}
+
 /*
  * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
  * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
@@ -845,6 +858,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
 	case FS_IOC_FSSETXATTR:
 		return ioctl_fssetxattr(filp, argp);
 
+	case FS_IOC_GETFSUUID:
+		return ioctl_getfsuuid(filp, argp);
+
 	default:
 		if (S_ISREG(inode->i_mode))
 			return file_ioctl(filp, cmd, argp);
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 48ad69f7722e..0389fea87db5 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -64,6 +64,20 @@ struct fstrim_range {
 	__u64 minlen;
 };
 
+/*
+ * We include a length field because some filesystems (vfat) have an identifier
+ * that we do want to expose as a UUID, but doesn't have the standard length.
+ *
+ * We use a fixed size buffer beacuse this interface will, by fiat, never
+ * support "UUIDs" longer than 16 bytes; we don't want to force all downstream
+ * users to have to deal with that.
+ */
+struct fsuuid2 {
+	__u32       fsu_len;
+	__u32       fsu_flags;
+	__u8        fsu_uuid[16];
+};
+
 /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
 #define FILE_DEDUPE_RANGE_SAME		0
 #define FILE_DEDUPE_RANGE_DIFFERS	1
@@ -215,6 +229,8 @@ struct fsxattr {
 #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
 #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
 #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
+#define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
+#define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
 
 /*
  * Inode flags (FS_IOC_GETFLAGS / FS_IOC_SETFLAGS)
-- 
2.43.0

[PATCH 3/6] fat: Hook up sb->s_uuid

[Kent Overstreet]

Now that we have a standard ioctl for querying the filesystem UUID,
initialize sb->s_uuid so that it works.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/fat/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 1fac3dabf130..a3d3478442d1 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1762,6 +1762,10 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	else /* fat 16 or 12 */
 		sbi->vol_id = bpb.fat16_vol_id;
 
+	__le32 vol_id_le = cpu_to_le32(sbi->vol_id);
+	memcpy(&sb->s_uuid, &vol_id_le, sizeof(vol_id_le));
+	sb->s_uuid_len = sizeof(vol_id_le);
+
 	sbi->dir_per_block = sb->s_blocksize / sizeof(struct msdos_dir_entry);
 	sbi->dir_per_block_bits = ffs(sbi->dir_per_block) - 1;
 
-- 
2.43.0

[PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Kent Overstreet]

Add a new ioctl for getting the sysfs name of a filesystem - the path
under /sys/fs.

This is going to let us standardize exporting data from sysfs across
filesystems, e.g. time stats.

The returned path will always be of the form "$FSTYP/$SYSFS_IDENTIFIER",
where the sysfs identifier may be a UUID (for bcachefs) or a device name
(xfs).

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Josef Bacik <josef@toxicpanda.com>
---
 fs/ioctl.c              | 17 +++++++++++++++++
 include/linux/fs.h      |  1 +
 include/uapi/linux/fs.h |  5 +++++
 3 files changed, 23 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 858801060408..cb3690811d3d 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -776,6 +776,20 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
 	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
 }
 
+static int ioctl_getfssysfsname(struct file *file, void __user *argp)
+{
+	struct super_block *sb = file_inode(file)->i_sb;
+
+	if (!strlen(sb->s_sysfs_name))
+		return -ENOIOCTLCMD;
+
+	struct fssysfsname u = {};
+
+	snprintf(u.name, sizeof(u.name), "%s/%s", sb->s_type->name, sb->s_sysfs_name);
+
+	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
+}
+
 /*
  * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
  * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
@@ -861,6 +875,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
 	case FS_IOC_GETFSUUID:
 		return ioctl_getfsuuid(filp, argp);
 
+	case FS_IOC_GETFSSYSFSNAME:
+		return ioctl_getfssysfsname(filp, argp);
+
 	default:
 		if (S_ISREG(inode->i_mode))
 			return file_ioctl(filp, cmd, argp);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ff41ea6c3a9c..7f23f593f17c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1258,6 +1258,7 @@ struct super_block {
 	char			s_id[32];	/* Informational name */
 	uuid_t			s_uuid;		/* UUID */
 	u8			s_uuid_len;	/* Default 16, possibly smaller for weird filesystems */
+	char			s_sysfs_name[UUID_STRING_LEN + 1];
 
 	unsigned int		s_max_links;
 
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 0389fea87db5..6dd14a453277 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -78,6 +78,10 @@ struct fsuuid2 {
 	__u8        fsu_uuid[16];
 };
 
+struct fssysfsname {
+	__u8			name[64];
+};
+
 /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
 #define FILE_DEDUPE_RANGE_SAME		0
 #define FILE_DEDUPE_RANGE_DIFFERS	1
@@ -231,6 +235,7 @@ struct fsxattr {
 #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
 #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
 #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
+#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
 
 /*
  * Inode flags (FS_IOC_GETFLAGS / FS_IOC_SETFLAGS)
-- 
2.43.0

[PATCH 5/6] xfs: add support for FS_IOC_GETSYSFSNAME

[Kent Overstreet]

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/xfs/xfs_mount.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index aabb25dc3efa..6d16203d5c1c 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -711,6 +711,8 @@ xfs_mountfs(
 	if (error)
 		goto out;
 
+	strscpy(mp->m_super->s_sysfs_name, mp->m_super->s_id, sizeof(mp->m_super->m_sysfs_name));
+
 	error = xfs_sysfs_init(&mp->m_stats.xs_kobj, &xfs_stats_ktype,
 			       &mp->m_kobj, "stats");
 	if (error)
-- 
2.43.0

[PATCH 6/6] bcachefs: add support for FS_IOC_GETSYSFSNAME

[Kent Overstreet]

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/bcachefs/fs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 77ea61090e91..50b2fd3ddd23 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1947,6 +1947,7 @@ static struct dentry *bch2_mount(struct file_system_type *fs_type,
 	sb->s_time_min		= div_s64(S64_MIN, c->sb.time_units_per_sec) + 1;
 	sb->s_time_max		= div_s64(S64_MAX, c->sb.time_units_per_sec);
 	sb->s_uuid		= c->sb.user_uuid;
+	snprintf(sb->s_sysfs_name, sizeof(sb->s_sysfs_name), "%pU", c->sb.user_uuid.b);
 	c->vfs_sb		= sb;
 	strscpy(sb->s_id, c->name, sizeof(sb->s_id));
 
-- 
2.43.0

Re: [PATCH 1/6] fs: super_block->s_uuid_len

[Dave Chinner]

On Mon, Feb 05, 2024 at 03:05:12PM -0500, Kent Overstreet wrote:
> Some weird old filesytems have UUID-like things that we wish to expose
> as UUIDs, but are smaller; add a length field so that the new
> FS_IOC_(GET|SET)UUID ioctls can handle them in generic code.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/super.c         | 1 +
>  include/linux/fs.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/super.c b/fs/super.c
> index d35e85295489..ed688d2a58a7 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -375,6 +375,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
>  	s->s_time_gran = 1000000000;
>  	s->s_time_min = TIME64_MIN;
>  	s->s_time_max = TIME64_MAX;
> +	s->s_uuid_len = sizeof(s->s_uuid);

So if the filesystem doesn't copy a uuid into sb->s_uuid, then we
allow those 16 bytes to be pulled from userspace?

Shouldn't this only get set when the filesystem copies it's uuid
to the superblock?

And then in the get uuid  ioctl, if s_uuid_len is zero we can return
-ENOENT to indicate the filesystem doesn't have a UUID, rather that
require userspace to determine a filesystem doesn't have a valid
UUID somehow...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/6] fs: FS_IOC_GETUUID

[Dave Chinner]

On Mon, Feb 05, 2024 at 03:05:13PM -0500, Kent Overstreet wrote:
> Add a new generic ioctls for querying the filesystem UUID.
> 
> These are lifted versions of the ext4 ioctls, with one change: we're not
> using a flexible array member, because UUIDs will never be more than 16
> bytes.
> 
> This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> reads from super_block->s_uuid; FS_IOC_SETFSUUID is left for individual
> filesystems to implement.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: linux-fsdevel@vger.kernel.or
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/ioctl.c              | 16 ++++++++++++++++
>  include/uapi/linux/fs.h | 16 ++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 76cf22ac97d7..858801060408 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
>  	return err;
>  }
>  
> +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> +{
> +	struct super_block *sb = file_inode(file)->i_sb;
> +
> +	if (WARN_ON(sb->s_uuid_len > sizeof(sb->s_uuid)))
> +		sb->s_uuid_len = sizeof(sb->s_uuid);

A "get"/read only ioctl should not be change superblock fields -
this is not the place for enforcing superblock filed constraints.
Make a helper function super_set_uuid(sb, uuid, uuid_len) for the
filesystems to call that does all the validity checking and then
sets the superblock fields appropriately.

> +
> +	struct fsuuid2 u = { .fsu_len = sb->s_uuid_len, };
> +	memcpy(&u.fsu_uuid[0], &sb->s_uuid, sb->s_uuid_len);

	if (!u.fsu_len)
		return -ENOENT;
	memcpy(&u.fsu_uuid[0], &sb->s_uuid, u.fsu_len);

> +
> +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> +}
> +
>  /*
>   * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
>   * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
> @@ -845,6 +858,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
>  	case FS_IOC_FSSETXATTR:
>  		return ioctl_fssetxattr(filp, argp);
>  
> +	case FS_IOC_GETFSUUID:
> +		return ioctl_getfsuuid(filp, argp);
> +
>  	default:
>  		if (S_ISREG(inode->i_mode))
>  			return file_ioctl(filp, cmd, argp);
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 48ad69f7722e..0389fea87db5 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -64,6 +64,20 @@ struct fstrim_range {
>  	__u64 minlen;
>  };
>  
> +/*
> + * We include a length field because some filesystems (vfat) have an identifier
> + * that we do want to expose as a UUID, but doesn't have the standard length.
> + *
> + * We use a fixed size buffer beacuse this interface will, by fiat, never
> + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream
> + * users to have to deal with that.
> + */
> +struct fsuuid2 {
> +	__u32       fsu_len;
> +	__u32       fsu_flags;
> +	__u8        fsu_uuid[16];
> +};

Nobody in userspace will care that this is "version 2" of the ext4
ioctl. I'd just name it "fs_uuid" as though the ext4 version didn't
ever exist.

> +
>  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
>  #define FILE_DEDUPE_RANGE_SAME		0
>  #define FILE_DEDUPE_RANGE_DIFFERS	1
> @@ -215,6 +229,8 @@ struct fsxattr {
>  #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
>  #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> +#define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> +#define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)

0x94 is the btrfs ioctl space, not the VFS space - why did you
choose that? That said, what is the VFS ioctl space identifier? 'v',
perhaps?

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Darrick J. Wong]

On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
> Add a new ioctl for getting the sysfs name of a filesystem - the path
> under /sys/fs.
> 
> This is going to let us standardize exporting data from sysfs across
> filesystems, e.g. time stats.
> 
> The returned path will always be of the form "$FSTYP/$SYSFS_IDENTIFIER",
> where the sysfs identifier may be a UUID (for bcachefs) or a device name
> (xfs).
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <djwong@kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/ioctl.c              | 17 +++++++++++++++++
>  include/linux/fs.h      |  1 +
>  include/uapi/linux/fs.h |  5 +++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 858801060408..cb3690811d3d 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -776,6 +776,20 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
>  	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
>  }
>  
> +static int ioctl_getfssysfsname(struct file *file, void __user *argp)

ackpthspacesplease.

"ioctl_get_fs_sysfs_name"?

> +{
> +	struct super_block *sb = file_inode(file)->i_sb;
> +
> +	if (!strlen(sb->s_sysfs_name))
> +		return -ENOIOCTLCMD;
> +
> +	struct fssysfsname u = {};
> +
> +	snprintf(u.name, sizeof(u.name), "%s/%s", sb->s_type->name, sb->s_sysfs_name);

Does this actually guarantee that there will be a trailing null in the
output?  It's really stupid that GETFSLABEL can return an unterminated
string if the label is exactly the size of the char array.

> +
> +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> +}
> +
>  /*
>   * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
>   * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
> @@ -861,6 +875,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
>  	case FS_IOC_GETFSUUID:
>  		return ioctl_getfsuuid(filp, argp);
>  
> +	case FS_IOC_GETFSSYSFSNAME:

File System Ioctl Get File System System File System Name.

Yuck.

FS_IOC_GETSYSFSPATH?

Also, do we want to establish that this works for /sys/fs and
/sys/kernel/debug at the same time?

> +		return ioctl_getfssysfsname(filp, argp);
> +
>  	default:
>  		if (S_ISREG(inode->i_mode))
>  			return file_ioctl(filp, cmd, argp);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ff41ea6c3a9c..7f23f593f17c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1258,6 +1258,7 @@ struct super_block {
>  	char			s_id[32];	/* Informational name */
>  	uuid_t			s_uuid;		/* UUID */
>  	u8			s_uuid_len;	/* Default 16, possibly smaller for weird filesystems */
> +	char			s_sysfs_name[UUID_STRING_LEN + 1];
>  
>  	unsigned int		s_max_links;
>  
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 0389fea87db5..6dd14a453277 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -78,6 +78,10 @@ struct fsuuid2 {
>  	__u8        fsu_uuid[16];
>  };
>  
> +struct fssysfsname {
> +	__u8			name[64];
> +};
> +
>  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
>  #define FILE_DEDUPE_RANGE_SAME		0
>  #define FILE_DEDUPE_RANGE_DIFFERS	1
> @@ -231,6 +235,7 @@ struct fsxattr {
>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
>  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
>  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)

0x94 is btrfs, don't add things to their "name" space.

--D

>  
>  /*
>   * Inode flags (FS_IOC_GETFLAGS / FS_IOC_SETFLAGS)
> -- 
> 2.43.0
> 
> 

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Kent Overstreet]

On Mon, Feb 05, 2024 at 02:27:32PM -0800, Darrick J. Wong wrote:
> On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
> > Add a new ioctl for getting the sysfs name of a filesystem - the path
> > under /sys/fs.
> > 
> > This is going to let us standardize exporting data from sysfs across
> > filesystems, e.g. time stats.
> > 
> > The returned path will always be of the form "$FSTYP/$SYSFS_IDENTIFIER",
> > where the sysfs identifier may be a UUID (for bcachefs) or a device name
> > (xfs).
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Dave Chinner <dchinner@redhat.com>
> > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/ioctl.c              | 17 +++++++++++++++++
> >  include/linux/fs.h      |  1 +
> >  include/uapi/linux/fs.h |  5 +++++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 858801060408..cb3690811d3d 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -776,6 +776,20 @@ static int ioctl_getfsuuid(struct file *file, void __user *argp)
> >  	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> >  }
> >  
> > +static int ioctl_getfssysfsname(struct file *file, void __user *argp)
> 
> ackpthspacesplease.
> 
> "ioctl_get_fs_sysfs_name"?

It did feel a bit trolling writing that :)

> 
> > +{
> > +	struct super_block *sb = file_inode(file)->i_sb;
> > +
> > +	if (!strlen(sb->s_sysfs_name))
> > +		return -ENOIOCTLCMD;
> > +
> > +	struct fssysfsname u = {};
> > +
> > +	snprintf(u.name, sizeof(u.name), "%s/%s", sb->s_type->name, sb->s_sysfs_name);
> 
> Does this actually guarantee that there will be a trailing null in the
> output?  It's really stupid that GETFSLABEL can return an unterminated
> string if the label is exactly the size of the char array.

It's snprintf, so yes.

(queue another "why are we using raw char arrays everywhere in 2024"
rant, I have to double check this stuff too).

> 
> > +
> > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > +}
> > +
> >  /*
> >   * do_vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
> >   * It's just a simple helper for sys_ioctl and compat_sys_ioctl.
> > @@ -861,6 +875,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> >  	case FS_IOC_GETFSUUID:
> >  		return ioctl_getfsuuid(filp, argp);
> >  
> > +	case FS_IOC_GETFSSYSFSNAME:
> 
> File System Ioctl Get File System System File System Name.
> 
> Yuck.
> 
> FS_IOC_GETSYSFSPATH?
> 
> Also, do we want to establish that this works for /sys/fs and
> /sys/kernel/debug at the same time?

Yeah, I'll add a comment to that effect.

> 
> > +		return ioctl_getfssysfsname(filp, argp);
> > +
> >  	default:
> >  		if (S_ISREG(inode->i_mode))
> >  			return file_ioctl(filp, cmd, argp);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index ff41ea6c3a9c..7f23f593f17c 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1258,6 +1258,7 @@ struct super_block {
> >  	char			s_id[32];	/* Informational name */
> >  	uuid_t			s_uuid;		/* UUID */
> >  	u8			s_uuid_len;	/* Default 16, possibly smaller for weird filesystems */
> > +	char			s_sysfs_name[UUID_STRING_LEN + 1];
> >  
> >  	unsigned int		s_max_links;
> >  
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index 0389fea87db5..6dd14a453277 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -78,6 +78,10 @@ struct fsuuid2 {
> >  	__u8        fsu_uuid[16];
> >  };
> >  
> > +struct fssysfsname {
> > +	__u8			name[64];
> > +};
> > +
> >  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
> >  #define FILE_DEDUPE_RANGE_SAME		0
> >  #define FILE_DEDUPE_RANGE_DIFFERS	1
> > @@ -231,6 +235,7 @@ struct fsxattr {
> >  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> >  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> >  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> > +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
> 
> 0x94 is btrfs, don't add things to their "name" space.

Can we please document this somewhere!?

What, dare I ask, is the "namespace" I should be using?

Re: [PATCH 2/6] fs: FS_IOC_GETUUID

[Kent Overstreet]

On Tue, Feb 06, 2024 at 09:17:58AM +1100, Dave Chinner wrote:
> On Mon, Feb 05, 2024 at 03:05:13PM -0500, Kent Overstreet wrote:
> > Add a new generic ioctls for querying the filesystem UUID.
> > 
> > These are lifted versions of the ext4 ioctls, with one change: we're not
> > using a flexible array member, because UUIDs will never be more than 16
> > bytes.
> > 
> > This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> > reads from super_block->s_uuid; FS_IOC_SETFSUUID is left for individual
> > filesystems to implement.
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Dave Chinner <dchinner@redhat.com>
> > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: linux-fsdevel@vger.kernel.or
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > ---
> >  fs/ioctl.c              | 16 ++++++++++++++++
> >  include/uapi/linux/fs.h | 16 ++++++++++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 76cf22ac97d7..858801060408 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
> >  	return err;
> >  }
> >  
> > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > +{
> > +	struct super_block *sb = file_inode(file)->i_sb;
> > +
> > +	if (WARN_ON(sb->s_uuid_len > sizeof(sb->s_uuid)))
> > +		sb->s_uuid_len = sizeof(sb->s_uuid);
> 
> A "get"/read only ioctl should not be change superblock fields -
> this is not the place for enforcing superblock filed constraints.
> Make a helper function super_set_uuid(sb, uuid, uuid_len) for the
> filesystems to call that does all the validity checking and then
> sets the superblock fields appropriately.

*nod* good thought...

> > +struct fsuuid2 {
> > +	__u32       fsu_len;
> > +	__u32       fsu_flags;
> > +	__u8        fsu_uuid[16];
> > +};
> 
> Nobody in userspace will care that this is "version 2" of the ext4
> ioctl. I'd just name it "fs_uuid" as though the ext4 version didn't
> ever exist.

I considered that - but I decided I wanted the explicit versioning,
because too often we live with unfixed mistakes because versioning is
ugly, or something?

Doing a new revision of an API should be a normal, frequent thing, and I
want to start making it a convention.

> 
> > +
> >  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
> >  #define FILE_DEDUPE_RANGE_SAME		0
> >  #define FILE_DEDUPE_RANGE_DIFFERS	1
> > @@ -215,6 +229,8 @@ struct fsxattr {
> >  #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
> >  #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
> >  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> > +#define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> > +#define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> 
> 0x94 is the btrfs ioctl space, not the VFS space - why did you
> choose that? That said, what is the VFS ioctl space identifier? 'v',
> perhaps?

"Promoting ioctls from fs to vfs without revising and renaming
considered harmful"... this is a mess that could have been avoided if we
weren't taking the lazy route.

And 'v' doesn't look like it to me, I really have no idea what to use
here. Does anyone?

Re: [PATCH 1/6] fs: super_block->s_uuid_len

[Kent Overstreet]

On Tue, Feb 06, 2024 at 08:58:49AM +1100, Dave Chinner wrote:
> On Mon, Feb 05, 2024 at 03:05:12PM -0500, Kent Overstreet wrote:
> > Some weird old filesytems have UUID-like things that we wish to expose
> > as UUIDs, but are smaller; add a length field so that the new
> > FS_IOC_(GET|SET)UUID ioctls can handle them in generic code.
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > ---
> >  fs/super.c         | 1 +
> >  include/linux/fs.h | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index d35e85295489..ed688d2a58a7 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -375,6 +375,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
> >  	s->s_time_gran = 1000000000;
> >  	s->s_time_min = TIME64_MIN;
> >  	s->s_time_max = TIME64_MAX;
> > +	s->s_uuid_len = sizeof(s->s_uuid);
> 
> So if the filesystem doesn't copy a uuid into sb->s_uuid, then we
> allow those 16 bytes to be pulled from userspace?
> 
> Shouldn't this only get set when the filesystem copies it's uuid
> to the superblock?
> 
> And then in the get uuid  ioctl, if s_uuid_len is zero we can return
> -ENOENT to indicate the filesystem doesn't have a UUID, rather that
> require userspace to determine a filesystem doesn't have a valid
> UUID somehow...

*nod* this all falls out of your super_set_uuid() suggestion

Re: [PATCH 2/6] fs: FS_IOC_GETUUID

[Darrick J. Wong]

On Mon, Feb 05, 2024 at 05:49:30PM -0500, Kent Overstreet wrote:
> On Tue, Feb 06, 2024 at 09:17:58AM +1100, Dave Chinner wrote:
> > On Mon, Feb 05, 2024 at 03:05:13PM -0500, Kent Overstreet wrote:
> > > Add a new generic ioctls for querying the filesystem UUID.
> > > 
> > > These are lifted versions of the ext4 ioctls, with one change: we're not
> > > using a flexible array member, because UUIDs will never be more than 16
> > > bytes.
> > > 
> > > This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> > > reads from super_block->s_uuid; FS_IOC_SETFSUUID is left for individual
> > > filesystems to implement.
> > > 
> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: Dave Chinner <dchinner@redhat.com>
> > > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > > Cc: Theodore Ts'o <tytso@mit.edu>
> > > Cc: linux-fsdevel@vger.kernel.or
> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > ---
> > >  fs/ioctl.c              | 16 ++++++++++++++++
> > >  include/uapi/linux/fs.h | 16 ++++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index 76cf22ac97d7..858801060408 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
> > >  	return err;
> > >  }
> > >  
> > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > +{
> > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > +
> > > +	if (WARN_ON(sb->s_uuid_len > sizeof(sb->s_uuid)))
> > > +		sb->s_uuid_len = sizeof(sb->s_uuid);
> > 
> > A "get"/read only ioctl should not be change superblock fields -
> > this is not the place for enforcing superblock filed constraints.
> > Make a helper function super_set_uuid(sb, uuid, uuid_len) for the
> > filesystems to call that does all the validity checking and then
> > sets the superblock fields appropriately.
> 
> *nod* good thought...
> 
> > > +struct fsuuid2 {
> > > +	__u32       fsu_len;
> > > +	__u32       fsu_flags;
> > > +	__u8        fsu_uuid[16];
> > > +};
> > 
> > Nobody in userspace will care that this is "version 2" of the ext4
> > ioctl. I'd just name it "fs_uuid" as though the ext4 version didn't
> > ever exist.
> 
> I considered that - but I decided I wanted the explicit versioning,
> because too often we live with unfixed mistakes because versioning is
> ugly, or something?
> 
> Doing a new revision of an API should be a normal, frequent thing, and I
> want to start making it a convention.
> 
> > 
> > > +
> > >  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
> > >  #define FILE_DEDUPE_RANGE_SAME		0
> > >  #define FILE_DEDUPE_RANGE_DIFFERS	1
> > > @@ -215,6 +229,8 @@ struct fsxattr {
> > >  #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
> > >  #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
> > >  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> > > +#define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> > > +#define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> > 
> > 0x94 is the btrfs ioctl space, not the VFS space - why did you
> > choose that? That said, what is the VFS ioctl space identifier? 'v',
> > perhaps?
> 
> "Promoting ioctls from fs to vfs without revising and renaming
> considered harmful"... this is a mess that could have been avoided if we
> weren't taking the lazy route.
> 
> And 'v' doesn't look like it to me, I really have no idea what to use
> here. Does anyone?

I thought it was 'f' but apparently that's ext?

--D

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[David Sterba]

On Mon, Feb 05, 2024 at 05:43:37PM -0500, Kent Overstreet wrote:
> On Mon, Feb 05, 2024 at 02:27:32PM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
> > > @@ -231,6 +235,7 @@ struct fsxattr {
> > >  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> > >  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> > >  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> > > +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
> > 
> > 0x94 is btrfs, don't add things to their "name" space.
> 
> Can we please document this somewhere!?
> 
> What, dare I ask, is the "namespace" I should be using?

Grep for _IOCTL_MAGIC in include/uapi:

uapi/linux/aspeed-lpc-ctrl.h:#define __ASPEED_LPC_CTRL_IOCTL_MAGIC 0xb2
uapi/linux/aspeed-p2a-ctrl.h:#define __ASPEED_P2A_CTRL_IOCTL_MAGIC 0xb3
uapi/linux/bt-bmc.h:#define __BT_BMC_IOCTL_MAGIC        0xb1
uapi/linux/btrfs.h:#define BTRFS_IOCTL_MAGIC 0x94
uapi/linux/f2fs.h:#define F2FS_IOCTL_MAGIC              0xf5
uapi/linux/ipmi_bmc.h:#define __IPMI_BMC_IOCTL_MAGIC        0xB1
uapi/linux/pfrut.h:#define PFRUT_IOCTL_MAGIC 0xEE
uapi/rdma/rdma_user_ioctl.h:#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
uapi/rdma/rdma_user_ioctl_cmds.h:#define RDMA_IOCTL_MAGIC       0x1b

The label ioctls inherited the 0x94 namespace for backward
compatibility but as already said, it's the private namespace of btrfs.

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Randy Dunlap]

On 2/5/24 17:39, David Sterba wrote:
> On Mon, Feb 05, 2024 at 05:43:37PM -0500, Kent Overstreet wrote:
>> On Mon, Feb 05, 2024 at 02:27:32PM -0800, Darrick J. Wong wrote:
>>> On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
>>>> @@ -231,6 +235,7 @@ struct fsxattr {
>>>>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
>>>>  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
>>>>  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
>>>> +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
>>>
>>> 0x94 is btrfs, don't add things to their "name" space.
>>
>> Can we please document this somewhere!?
>>
>> What, dare I ask, is the "namespace" I should be using?
> 
> Grep for _IOCTL_MAGIC in include/uapi:
> 
> uapi/linux/aspeed-lpc-ctrl.h:#define __ASPEED_LPC_CTRL_IOCTL_MAGIC 0xb2
> uapi/linux/aspeed-p2a-ctrl.h:#define __ASPEED_P2A_CTRL_IOCTL_MAGIC 0xb3
> uapi/linux/bt-bmc.h:#define __BT_BMC_IOCTL_MAGIC        0xb1
> uapi/linux/btrfs.h:#define BTRFS_IOCTL_MAGIC 0x94
> uapi/linux/f2fs.h:#define F2FS_IOCTL_MAGIC              0xf5
> uapi/linux/ipmi_bmc.h:#define __IPMI_BMC_IOCTL_MAGIC        0xB1
> uapi/linux/pfrut.h:#define PFRUT_IOCTL_MAGIC 0xEE
> uapi/rdma/rdma_user_ioctl.h:#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
> uapi/rdma/rdma_user_ioctl_cmds.h:#define RDMA_IOCTL_MAGIC       0x1b
> 
> The label ioctls inherited the 0x94 namespace for backward
> compatibility but as already said, it's the private namespace of btrfs.
> 

or more generally, see Documentation/userspace-api/ioctl/ioctl-number.rst.

For 0x94, it says:

0x94  all    fs/btrfs/ioctl.h                                        Btrfs filesystem
             and linux/fs.h                                          some lifted to vfs/generic

-- 
#Randy

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Kent Overstreet]

On Mon, Feb 05, 2024 at 08:20:10PM -0800, Randy Dunlap wrote:
> 
> 
> On 2/5/24 17:39, David Sterba wrote:
> > On Mon, Feb 05, 2024 at 05:43:37PM -0500, Kent Overstreet wrote:
> >> On Mon, Feb 05, 2024 at 02:27:32PM -0800, Darrick J. Wong wrote:
> >>> On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
> >>>> @@ -231,6 +235,7 @@ struct fsxattr {
> >>>>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> >>>>  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> >>>>  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> >>>> +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
> >>>
> >>> 0x94 is btrfs, don't add things to their "name" space.
> >>
> >> Can we please document this somewhere!?
> >>
> >> What, dare I ask, is the "namespace" I should be using?
> > 
> > Grep for _IOCTL_MAGIC in include/uapi:
> > 
> > uapi/linux/aspeed-lpc-ctrl.h:#define __ASPEED_LPC_CTRL_IOCTL_MAGIC 0xb2
> > uapi/linux/aspeed-p2a-ctrl.h:#define __ASPEED_P2A_CTRL_IOCTL_MAGIC 0xb3
> > uapi/linux/bt-bmc.h:#define __BT_BMC_IOCTL_MAGIC        0xb1
> > uapi/linux/btrfs.h:#define BTRFS_IOCTL_MAGIC 0x94
> > uapi/linux/f2fs.h:#define F2FS_IOCTL_MAGIC              0xf5
> > uapi/linux/ipmi_bmc.h:#define __IPMI_BMC_IOCTL_MAGIC        0xB1
> > uapi/linux/pfrut.h:#define PFRUT_IOCTL_MAGIC 0xEE
> > uapi/rdma/rdma_user_ioctl.h:#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
> > uapi/rdma/rdma_user_ioctl_cmds.h:#define RDMA_IOCTL_MAGIC       0x1b
> > 
> > The label ioctls inherited the 0x94 namespace for backward
> > compatibility but as already said, it's the private namespace of btrfs.
> > 
> 
> or more generally, see Documentation/userspace-api/ioctl/ioctl-number.rst.
> 
> For 0x94, it says:
> 
> 0x94  all    fs/btrfs/ioctl.h                                        Btrfs filesystem
>              and linux/fs.h                                          some lifted to vfs/generic

You guys keep giving the same info over and over again, instead of
anything that would be actually helpful...

Does anyone know what the proper "namespace" is for new VFS level
ioctls?

...Anyone?

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Darrick J. Wong]

On Mon, Feb 05, 2024 at 11:33:11PM -0500, Kent Overstreet wrote:
> On Mon, Feb 05, 2024 at 08:20:10PM -0800, Randy Dunlap wrote:
> > 
> > 
> > On 2/5/24 17:39, David Sterba wrote:
> > > On Mon, Feb 05, 2024 at 05:43:37PM -0500, Kent Overstreet wrote:
> > >> On Mon, Feb 05, 2024 at 02:27:32PM -0800, Darrick J. Wong wrote:
> > >>> On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
> > >>>> @@ -231,6 +235,7 @@ struct fsxattr {
> > >>>>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> > >>>>  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> > >>>>  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> > >>>> +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
> > >>>
> > >>> 0x94 is btrfs, don't add things to their "name" space.
> > >>
> > >> Can we please document this somewhere!?
> > >>
> > >> What, dare I ask, is the "namespace" I should be using?
> > > 
> > > Grep for _IOCTL_MAGIC in include/uapi:
> > > 
> > > uapi/linux/aspeed-lpc-ctrl.h:#define __ASPEED_LPC_CTRL_IOCTL_MAGIC 0xb2
> > > uapi/linux/aspeed-p2a-ctrl.h:#define __ASPEED_P2A_CTRL_IOCTL_MAGIC 0xb3
> > > uapi/linux/bt-bmc.h:#define __BT_BMC_IOCTL_MAGIC        0xb1
> > > uapi/linux/btrfs.h:#define BTRFS_IOCTL_MAGIC 0x94
> > > uapi/linux/f2fs.h:#define F2FS_IOCTL_MAGIC              0xf5
> > > uapi/linux/ipmi_bmc.h:#define __IPMI_BMC_IOCTL_MAGIC        0xB1
> > > uapi/linux/pfrut.h:#define PFRUT_IOCTL_MAGIC 0xEE
> > > uapi/rdma/rdma_user_ioctl.h:#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
> > > uapi/rdma/rdma_user_ioctl_cmds.h:#define RDMA_IOCTL_MAGIC       0x1b
> > > 
> > > The label ioctls inherited the 0x94 namespace for backward
> > > compatibility but as already said, it's the private namespace of btrfs.
> > > 
> > 
> > or more generally, see Documentation/userspace-api/ioctl/ioctl-number.rst.
> > 
> > For 0x94, it says:
> > 
> > 0x94  all    fs/btrfs/ioctl.h                                        Btrfs filesystem
> >              and linux/fs.h                                          some lifted to vfs/generic
> 
> You guys keep giving the same info over and over again, instead of
> anything that would be actually helpful...
> 
> Does anyone know what the proper "namespace" is for new VFS level
> ioctls?
> 
> ...Anyone?

I propose you use 0x15 (NAK) and add it to the Documentation/ as the
official VFS ioctl namespace. ;)

--D

Re: [PATCH 4/6] fs: FS_IOC_GETSYSFSNAME

[Kent Overstreet]

On Mon, Feb 05, 2024 at 09:08:53PM -0800, Darrick J. Wong wrote:
> On Mon, Feb 05, 2024 at 11:33:11PM -0500, Kent Overstreet wrote:
> > On Mon, Feb 05, 2024 at 08:20:10PM -0800, Randy Dunlap wrote:
> > > 
> > > 
> > > On 2/5/24 17:39, David Sterba wrote:
> > > > On Mon, Feb 05, 2024 at 05:43:37PM -0500, Kent Overstreet wrote:
> > > >> On Mon, Feb 05, 2024 at 02:27:32PM -0800, Darrick J. Wong wrote:
> > > >>> On Mon, Feb 05, 2024 at 03:05:15PM -0500, Kent Overstreet wrote:
> > > >>>> @@ -231,6 +235,7 @@ struct fsxattr {
> > > >>>>  #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
> > > >>>>  #define FS_IOC_GETFSUUID		_IOR(0x94, 51, struct fsuuid2)
> > > >>>>  #define FS_IOC_SETFSUUID		_IOW(0x94, 52, struct fsuuid2)
> > > >>>> +#define FS_IOC_GETFSSYSFSNAME		_IOR(0x94, 53, struct fssysfsname)
> > > >>>
> > > >>> 0x94 is btrfs, don't add things to their "name" space.
> > > >>
> > > >> Can we please document this somewhere!?
> > > >>
> > > >> What, dare I ask, is the "namespace" I should be using?
> > > > 
> > > > Grep for _IOCTL_MAGIC in include/uapi:
> > > > 
> > > > uapi/linux/aspeed-lpc-ctrl.h:#define __ASPEED_LPC_CTRL_IOCTL_MAGIC 0xb2
> > > > uapi/linux/aspeed-p2a-ctrl.h:#define __ASPEED_P2A_CTRL_IOCTL_MAGIC 0xb3
> > > > uapi/linux/bt-bmc.h:#define __BT_BMC_IOCTL_MAGIC        0xb1
> > > > uapi/linux/btrfs.h:#define BTRFS_IOCTL_MAGIC 0x94
> > > > uapi/linux/f2fs.h:#define F2FS_IOCTL_MAGIC              0xf5
> > > > uapi/linux/ipmi_bmc.h:#define __IPMI_BMC_IOCTL_MAGIC        0xB1
> > > > uapi/linux/pfrut.h:#define PFRUT_IOCTL_MAGIC 0xEE
> > > > uapi/rdma/rdma_user_ioctl.h:#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC
> > > > uapi/rdma/rdma_user_ioctl_cmds.h:#define RDMA_IOCTL_MAGIC       0x1b
> > > > 
> > > > The label ioctls inherited the 0x94 namespace for backward
> > > > compatibility but as already said, it's the private namespace of btrfs.
> > > > 
> > > 
> > > or more generally, see Documentation/userspace-api/ioctl/ioctl-number.rst.
> > > 
> > > For 0x94, it says:
> > > 
> > > 0x94  all    fs/btrfs/ioctl.h                                        Btrfs filesystem
> > >              and linux/fs.h                                          some lifted to vfs/generic
> > 
> > You guys keep giving the same info over and over again, instead of
> > anything that would be actually helpful...
> > 
> > Does anyone know what the proper "namespace" is for new VFS level
> > ioctls?
> > 
> > ...Anyone?
> 
> I propose you use 0x15 (NAK) and add it to the Documentation/ as the
> official VFS ioctl namespace. ;)

Done!

Re: [PATCH 2/6] fs: FS_IOC_GETUUID

[Amir Goldstein]

On Tue, Feb 6, 2024 at 12:49 AM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> On Tue, Feb 06, 2024 at 09:17:58AM +1100, Dave Chinner wrote:
> > On Mon, Feb 05, 2024 at 03:05:13PM -0500, Kent Overstreet wrote:
> > > Add a new generic ioctls for querying the filesystem UUID.
> > >
> > > These are lifted versions of the ext4 ioctls, with one change: we're not
> > > using a flexible array member, because UUIDs will never be more than 16
> > > bytes.
> > >
> > > This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> > > reads from super_block->s_uuid; FS_IOC_SETFSUUID is left for individual
> > > filesystems to implement.
> > >

It's fine to have a generic implementation, but the filesystem should
have the option to opt-in for a specific implementation.

There are several examples, even with xfs and btrfs where ->s_uuid
does not contain the filesystem's UUID or there is more than one
uuid and ->s_uuid is not the correct one to expose to the user.

A model like ioctl_[gs]etflags() looks much more appropriate
and could be useful for network filesystems/FUSE as well.

> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: Dave Chinner <dchinner@redhat.com>
> > > Cc: "Darrick J. Wong" <djwong@kernel.org>
> > > Cc: Theodore Ts'o <tytso@mit.edu>
> > > Cc: linux-fsdevel@vger.kernel.or
> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > ---
> > >  fs/ioctl.c              | 16 ++++++++++++++++
> > >  include/uapi/linux/fs.h | 16 ++++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > >
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index 76cf22ac97d7..858801060408 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -763,6 +763,19 @@ static int ioctl_fssetxattr(struct file *file, void __user *argp)
> > >     return err;
> > >  }
> > >
> > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > +{
> > > +   struct super_block *sb = file_inode(file)->i_sb;
> > > +
> > > +   if (WARN_ON(sb->s_uuid_len > sizeof(sb->s_uuid)))
> > > +           sb->s_uuid_len = sizeof(sb->s_uuid);
> >
> > A "get"/read only ioctl should not be change superblock fields -
> > this is not the place for enforcing superblock filed constraints.
> > Make a helper function super_set_uuid(sb, uuid, uuid_len) for the
> > filesystems to call that does all the validity checking and then
> > sets the superblock fields appropriately.
>
> *nod* good thought...
>
> > > +struct fsuuid2 {
> > > +   __u32       fsu_len;
> > > +   __u32       fsu_flags;
> > > +   __u8        fsu_uuid[16];
> > > +};
> >
> > Nobody in userspace will care that this is "version 2" of the ext4
> > ioctl. I'd just name it "fs_uuid" as though the ext4 version didn't
> > ever exist.
>
> I considered that - but I decided I wanted the explicit versioning,
> because too often we live with unfixed mistakes because versioning is
> ugly, or something?
>
> Doing a new revision of an API should be a normal, frequent thing, and I
> want to start making it a convention.
>
> >
> > > +
> > >  /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
> > >  #define FILE_DEDUPE_RANGE_SAME             0
> > >  #define FILE_DEDUPE_RANGE_DIFFERS  1
> > > @@ -215,6 +229,8 @@ struct fsxattr {
> > >  #define FS_IOC_FSSETXATTR          _IOW('X', 32, struct fsxattr)
> > >  #define FS_IOC_GETFSLABEL          _IOR(0x94, 49, char[FSLABEL_MAX])
> > >  #define FS_IOC_SETFSLABEL          _IOW(0x94, 50, char[FSLABEL_MAX])
> > > +#define FS_IOC_GETFSUUID           _IOR(0x94, 51, struct fsuuid2)
> > > +#define FS_IOC_SETFSUUID           _IOW(0x94, 52, struct fsuuid2)
> >
> > 0x94 is the btrfs ioctl space, not the VFS space - why did you
> > choose that? That said, what is the VFS ioctl space identifier? 'v',
> > perhaps?
>
> "Promoting ioctls from fs to vfs without revising and renaming
> considered harmful"... this is a mess that could have been avoided if we
> weren't taking the lazy route.
>
> And 'v' doesn't look like it to me, I really have no idea what to use
> here. Does anyone?
>

All the other hoisted FS_IOC_* use the original fs ioctl namespace they
came from. Although it is not an actual hoist, I'd use:

struct fsuuid128 {
       __u32       fsu_len;
       __u32       fsu_flags;
       __u8        fsu_uuid[16];
};

#define FS_IOC_GETFSUUID              _IOR('f', 45, struct fsuuid128)
#define FS_IOC_SETFSUUID              _IOW('f', 46, struct fsuuid128)

Technically, could also overload EXT4_IOC_[GS]ETFSUUID numbers
because of the different type:

#define FS_IOC_GETFSUUID              _IOR('f', 44, struct fsuuid128)
#define FS_IOC_SETFSUUID              _IOW('f', 44, struct fsuuid128)

and then ext4 can follow up with this patch, because as far as I can tell,
the ext4 implementation is already compatible with the new ioctls.

Thanks,
Amir.

--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1613,8 +1613,10 @@ static long __ext4_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
                return ext4_ioctl_setlabel(filp,
                                           (const void __user *)arg);

+       case FS_IOC_GETFSUUID:
         case EXT4_IOC_GETFSUUID:
                 return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg);
+       case FS_IOC_SETFSUUID:
         case EXT4_IOC_SETFSUUID:
                 return ext4_ioctl_setuuid(filp, (const void __user *)arg);

Re: [PATCH 2/6] fs: FS_IOC_GETUUID

[Kent Overstreet]

On Tue, Feb 06, 2024 at 10:24:45AM +0200, Amir Goldstein wrote:
> On Tue, Feb 6, 2024 at 12:49 AM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > On Tue, Feb 06, 2024 at 09:17:58AM +1100, Dave Chinner wrote:
> > > On Mon, Feb 05, 2024 at 03:05:13PM -0500, Kent Overstreet wrote:
> > > > Add a new generic ioctls for querying the filesystem UUID.
> > > >
> > > > These are lifted versions of the ext4 ioctls, with one change: we're not
> > > > using a flexible array member, because UUIDs will never be more than 16
> > > > bytes.
> > > >
> > > > This patch adds a generic implementation of FS_IOC_GETFSUUID, which
> > > > reads from super_block->s_uuid; FS_IOC_SETFSUUID is left for individual
> > > > filesystems to implement.
> > > >
> 
> It's fine to have a generic implementation, but the filesystem should
> have the option to opt-in for a specific implementation.
> 
> There are several examples, even with xfs and btrfs where ->s_uuid
> does not contain the filesystem's UUID or there is more than one
> uuid and ->s_uuid is not the correct one to expose to the user.

Yeah, some of you were smoking some good stuff from the stories I've
been hearing...

> A model like ioctl_[gs]etflags() looks much more appropriate
> and could be useful for network filesystems/FUSE as well.

A filesystem needs to store two UUIDs (that identify the filesystem as a
whole).

 - Your internal UUID, which can never change because it's referenced in
   various other on disk data structures
 - Your external UUID, which identifies the filesystem to the outside
   world. Users want to be able to change this - which is why it has to
   be distinct from the internal UUID.

The internal UUID must never be exposed to the outside world, and that
includes the VFS; storing your private UUID in sb->s_uuid is wrong -
separation of concerns.

yes, I am aware of fscrypt, and yes, someone's going to have to fix
that.

This interface is only for the external/public UUID.

Re: [PATCH 0/6] filesystem visibility ioctls

[Christian Brauner]

On Mon, Feb 05, 2024 at 03:05:11PM -0500, Kent Overstreet wrote:
> Hi all,
> 
> this patchset adds a few new ioctls to standardize a few interfaces we
> want
>  - get/set UUID

Last time I spoke in favor of exposing the UUID as a generic ioctl most
were supportive. But I remember that setting the UUID was a lot more
contentious. If that's changed though then great.

>  - get sysfs path
> 
> The get/set UUID ioctls are lifted versions of the ext4 ioctls with one
> difference, killing the flexible array member - we'll never have UUIDs
> more than 16 bytes, and getting rid of the flexible array member makes
> them easier to use.
> 
> FS_IOC_GETSYSFSNAME is new, but it addresses something that we've been
> doing in fs specific code for awhile - "given a path on a mounted
> filesystem, tell me where it lives in sysfs".
> 
> Cheers,
> Kent

When you send v2 could you please just put me in to. Makes it easier for
me to pick this series from the list. Thanks!