[PATCH v2 0/7] filesystem visibililty ioctls

[PATCH v2 0/7] filesystem visibililty ioctls

[Kent Overstreet]

previous:
https://lore.kernel.org/linux-fsdevel/20240206-aufwuchs-atomkraftgegner-dc53ce1e435f@brauner/T/

Changes since v1:
 - super_set_uuid() helper, per Dave

 - nix FS_IOC_SETUUID - Al raised this and I'm in 100% agreement,
   changing a UUID on an existing filesystem is a rare operation that
   should only be done when the filesystem is offline; we'd need to
   audit/fix a bunch of stuff if we wanted to support this

 - fix iocl numberisng, no longer using btrfs's space

 - flags argument in struct fsuuid2 is gone; since we're no longer
   setting this is no longer needed. As discussed previously, this
   interface is only for exporting the public, user-changable UUID (and
   there's now a comment saying that this exports the same UUID that
   libblkid reports, per Darrick).

Darrick also noticed that fscrypt (!) is using sb->s_uuid, which looks
busted - they want to be using the "this can never change" UUID, but
that is not an item for this patchset.

 - FS_IOC_GETSYSFSNAME -> FS_IOC_GETSYSFSPATH, per Darrick (the commit
   messages didn't get updated, whoops); and there's now a comment to
   reflect that this patch is also for finding filesystem info under
   debugfs, if present.

Christain, if nothing else comes up, are you ready to take this?

Cheers,
Kent

Kent Overstreet (7):
  fs: super_set_uuid()
  overlayfs: Convert to super_set_uuid()
  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        |  3 ++-
 fs/ext4/super.c         |  2 +-
 fs/f2fs/super.c         |  2 +-
 fs/fat/inode.c          |  3 +++
 fs/gfs2/ops_fstype.c    |  2 +-
 fs/ioctl.c              | 33 +++++++++++++++++++++++++++++++++
 fs/kernfs/mount.c       |  4 +++-
 fs/ocfs2/super.c        |  4 ++--
 fs/overlayfs/util.c     | 14 +++++++++-----
 fs/ubifs/super.c        |  2 +-
 fs/xfs/xfs_mount.c      |  4 +++-
 include/linux/fs.h      | 10 ++++++++++
 include/uapi/linux/fs.h | 27 +++++++++++++++++++++++++++
 mm/shmem.c              |  4 +++-
 14 files changed, 99 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 1/7] fs: super_set_uuid()

[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.

And add a helper super_set_uuid(), for setting nonstandard length uuids.

Helper is now required for the new FS_IOC_GETUUID ioctl; if
super_set_uuid() hasn't been called, the ioctl won't be supported.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/bcachefs/fs.c     | 2 +-
 fs/ext4/super.c      | 2 +-
 fs/f2fs/super.c      | 2 +-
 fs/gfs2/ops_fstype.c | 2 +-
 fs/kernfs/mount.c    | 4 +++-
 fs/ocfs2/super.c     | 4 ++--
 fs/ubifs/super.c     | 2 +-
 fs/xfs/xfs_mount.c   | 2 +-
 include/linux/fs.h   | 9 +++++++++
 mm/shmem.c           | 4 +++-
 10 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 77ea61090e91..68e9a89e42bb 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -1946,7 +1946,7 @@ static struct dentry *bch2_mount(struct file_system_type *fs_type,
 	sb->s_time_gran		= c->sb.nsec_per_time_unit;
 	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;
+	super_set_uuid(sb, c->sb.user_uuid.b, sizeof(c->sb.user_uuid));
 	c->vfs_sb		= sb;
 	strscpy(sb->s_id, c->name, sizeof(sb->s_id));
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dcba0f85dfe2..9e28ebd0869a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5346,7 +5346,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
 		sb->s_qcop = &ext4_qctl_operations;
 	sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP | QTYPE_MASK_PRJ;
 #endif
-	memcpy(&sb->s_uuid, es->s_uuid, sizeof(es->s_uuid));
+	super_set_uuid(sb, es->s_uuid, sizeof(es->s_uuid));
 
 	INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
 	mutex_init(&sbi->s_orphan_lock);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index d45ab0992ae5..5dd7b7b26db9 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4496,7 +4496,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_time_gran = 1;
 	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
 		(test_opt(sbi, POSIX_ACL) ? SB_POSIXACL : 0);
-	memcpy(&sb->s_uuid, raw_super->uuid, sizeof(raw_super->uuid));
+	super_set_uuid(&sb, (void *) raw_super->uuid, sizeof(raw_super->uuid));
 	sb->s_iflags |= SB_I_CGROUPWB;
 
 	/* init f2fs-specific super block info */
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 1281e60be639..572d58e86296 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -214,7 +214,7 @@ static void gfs2_sb_in(struct gfs2_sbd *sdp, const void *buf)
 
 	memcpy(sb->sb_lockproto, str->sb_lockproto, GFS2_LOCKNAME_LEN);
 	memcpy(sb->sb_locktable, str->sb_locktable, GFS2_LOCKNAME_LEN);
-	memcpy(&s->s_uuid, str->sb_uuid, 16);
+	super_set_uuid(s, str->sb_uuid, 16);
 }
 
 /**
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index 0c93cad0f0ac..e29f4edf9572 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -358,7 +358,9 @@ int kernfs_get_tree(struct fs_context *fc)
 		}
 		sb->s_flags |= SB_ACTIVE;
 
-		uuid_gen(&sb->s_uuid);
+		uuid_t uuid;
+		uuid_gen(&uuid);
+		super_set_uuid(sb, uuid.b, sizeof(uuid));
 
 		down_write(&root->kernfs_supers_rwsem);
 		list_add(&info->node, &info->root->supers);
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 6b906424902b..a70aff17d455 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2027,8 +2027,8 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	cbits = le32_to_cpu(di->id2.i_super.s_clustersize_bits);
 	bbits = le32_to_cpu(di->id2.i_super.s_blocksize_bits);
 	sb->s_maxbytes = ocfs2_max_file_offset(bbits, cbits);
-	memcpy(&sb->s_uuid, di->id2.i_super.s_uuid,
-	       sizeof(di->id2.i_super.s_uuid));
+	super_set_uuid(sb, di->id2.i_super.s_uuid,
+		       sizeof(di->id2.i_super.s_uuid));
 
 	osb->osb_dx_mask = (1 << (cbits - bbits)) - 1;
 
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 09e270d6ed02..f780729eec06 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2245,7 +2245,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 		goto out_umount;
 	}
 
-	import_uuid(&sb->s_uuid, c->uuid);
+	super_set_uuid(sb, c->uuid, sizeof(c->uuid));
 
 	mutex_unlock(&c->umount_mutex);
 	return 0;
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index aabb25dc3efa..4a46bc44088f 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -62,7 +62,7 @@ xfs_uuid_mount(
 	int			hole, i;
 
 	/* Publish UUID in struct super_block */
-	uuid_copy(&mp->m_super->s_uuid, uuid);
+	super_set_uuid(mp->m_super, uuid->b, sizeof(*uuid));
 
 	if (xfs_has_nouuid(mp))
 		return 0;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ed5966a70495..acdc56987cb1 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;
 
@@ -2532,6 +2533,14 @@ extern __printf(2, 3)
 int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
 extern int super_setup_bdi(struct super_block *sb);
 
+static inline void super_set_uuid(struct super_block *sb, const u8 *uuid, unsigned len)
+{
+	if (WARN_ON(len > sizeof(sb->s_uuid)))
+		len = sizeof(sb->s_uuid);
+	sb->s_uuid_len = len;
+	memcpy(&sb->s_uuid, uuid, len);
+}
+
 extern int current_umask(void);
 
 extern void ihold(struct inode * inode);
diff --git a/mm/shmem.c b/mm/shmem.c
index d7c84ff62186..be41955e52da 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4355,7 +4355,9 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
 #ifdef CONFIG_TMPFS_POSIX_ACL
 	sb->s_flags |= SB_POSIXACL;
 #endif
-	uuid_gen(&sb->s_uuid);
+	uuid_t uuid;
+	uuid_gen(&uuid);
+	super_set_uuid(sb, uuid.b, sizeof(uuid));
 
 #ifdef CONFIG_TMPFS_QUOTA
 	if (ctx->seen & SHMEM_SEEN_QUOTA) {
-- 
2.43.0

[PATCH v2 2/7] overlayfs: Convert to super_set_uuid()

[Kent Overstreet]

We don't want to be settingc sb->s_uuid directly anymore, as there's a
length field that also has to be set, and this conversion was not
completely trivial.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: linux-unionfs@vger.kernel.org
---
 fs/overlayfs/util.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0217094c23ea..f1f0ee9a9dff 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
 			 const struct path *upperpath)
 {
 	bool set = false;
+	uuid_t uuid;
 	int res;
 
 	/* Try to load existing persistent uuid */
-	res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
+	res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
 				UUID_SIZE);
 	if (res == UUID_SIZE)
-		return true;
+		goto success;
 
 	if (res != -ENODATA)
 		goto fail;
@@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
 	}
 
 	/* Generate overlay instance uuid */
-	uuid_gen(&sb->s_uuid);
+	uuid_gen(&uuid);
 
 	/* Try to store persistent uuid */
 	set = true;
-	res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
+	res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
 			   UUID_SIZE);
 	if (res == 0)
-		return true;
+		goto success;
 
 fail:
 	memset(sb->s_uuid.b, 0, UUID_SIZE);
@@ -809,6 +810,9 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
 	pr_warn("failed to %s uuid (%pd2, err=%i); falling back to uuid=null.\n",
 		set ? "set" : "get", upperpath->dentry, res);
 	return false;
+success:
+	super_set_uuid(sb, uuid.b, sizeof(uuid));
+	return true;
 }
 
 bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path,
-- 
2.43.0

[PATCH v2 3/7] 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. We're not lifting SETFSUUID from ext4 -
that can be done on offline filesystems by the people who need it,
trying to do it online is just asking for too much trouble.

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 | 17 +++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..046c30294a82 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 (!sb->s_uuid_len)
+		return -ENOIOCTLCMD;
+
+	struct fsuuid2 u = { .len = sb->s_uuid_len, };
+	memcpy(&u.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..16a6ecadfd8d 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -64,6 +64,19 @@ 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 {
+	__u8	len;
+	__u8	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
@@ -190,6 +203,9 @@ struct fsxattr {
  * (see uapi/linux/blkzoned.h)
  */
 
+/* Returns the external filesystem UUID, the same one blkid returns */
+#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
+
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
@@ -198,6 +214,7 @@ struct fsxattr {
 #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
 #define FICLONE		_IOW(0x94, 9, int)
 #define FICLONERANGE	_IOW(0x94, 13, struct file_clone_range)
+
 #define FIDEDUPERANGE	_IOWR(0x94, 54, struct file_dedupe_range)
 
 #define FSLABEL_MAX 256	/* Max chars for the interface; each fs may differ */
-- 
2.43.0

[PATCH v2 4/7] 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 1fac3dabf130..5c813696d1ff 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1762,6 +1762,9 @@ 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);
+	super_set_uuid(sb, (void *) &vol_id_le, 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 v2 5/7] 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).

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>
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/ioctl.c              | 17 +++++++++++++++++
 include/linux/fs.h      |  1 +
 include/uapi/linux/fs.h | 10 ++++++++++
 3 files changed, 28 insertions(+)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 046c30294a82..7c37765bd04e 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_get_fs_sysfs_path(struct file *file, void __user *argp)
+{
+	struct super_block *sb = file_inode(file)->i_sb;
+
+	if (!strlen(sb->s_sysfs_name))
+		return -ENOIOCTLCMD;
+
+	struct fssysfspath 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_GETFSSYSFSPATH:
+		return ioctl_get_fs_sysfs_path(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 acdc56987cb1..b96c1d009718 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 16a6ecadfd8d..c0f7bd4b6350 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -77,6 +77,10 @@ struct fsuuid2 {
 	__u8	uuid[16];
 };
 
+struct fssysfspath {
+	__u8			name[128];
+};
+
 /* extent-same (dedupe) ioctls; these MUST match the btrfs ioctl definitions */
 #define FILE_DEDUPE_RANGE_SAME		0
 #define FILE_DEDUPE_RANGE_DIFFERS	1
@@ -206,6 +210,12 @@ struct fsxattr {
 /* Returns the external filesystem UUID, the same one blkid returns */
 #define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
 
+/*
+ * Returns the path component under /sys/fs/ that refers to this filesystem;
+ * also /sys/kernel/debug/ for filesystems with debugfs exports
+ */
+#define FS_IOC_GETFSSYSFSPATH		_IOR(0x12, 143, struct fssysfspath)
+
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
 #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
-- 
2.43.0

[PATCH v2 6/7] 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 4a46bc44088f..d87d14d8f699 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 v2 7/7] 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 68e9a89e42bb..785438dbe7ef 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);
 	super_set_uuid(sb, c->sb.user_uuid.b, sizeof(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 v2 3/7] fs: FS_IOC_GETUUID

[Randy Dunlap]

On 2/6/24 12:18, 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. We're not lifting SETFSUUID from ext4 -
> that can be done on offline filesystems by the people who need it,
> trying to do it online is just asking for too much trouble.
> 
> 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 | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+)
> 


> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 48ad69f7722e..16a6ecadfd8d 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -64,6 +64,19 @@ 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

                                 because

> + * support "UUIDs" longer than 16 bytes; we don't want to force all downstream
> + * users to have to deal with that.
> + */
> +struct fsuuid2 {
> +	__u8	len;
> +	__u8	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
> @@ -190,6 +203,9 @@ struct fsxattr {
>   * (see uapi/linux/blkzoned.h)
>   */
>  
> +/* Returns the external filesystem UUID, the same one blkid returns */
> +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> +
>  #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
>  #define FIBMAP	   _IO(0x00,1)	/* bmap access */
>  #define FIGETBSZ   _IO(0x00,2)	/* get the block size used for bmap */
> @@ -198,6 +214,7 @@ struct fsxattr {
>  #define FITRIM		_IOWR('X', 121, struct fstrim_range)	/* Trim */
>  #define FICLONE		_IOW(0x94, 9, int)
>  #define FICLONERANGE	_IOW(0x94, 13, struct file_clone_range)
> +
>  #define FIDEDUPERANGE	_IOWR(0x94, 54, struct file_dedupe_range)

Why the additional blank line? (nit)

>  
>  #define FSLABEL_MAX 256	/* Max chars for the interface; each fs may differ */

-- 
#Randy

Re: [PATCH v2 1/7] fs: super_set_uuid()

[Dave Chinner]

On Tue, Feb 06, 2024 at 03:18:49PM -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.
> 
> And add a helper super_set_uuid(), for setting nonstandard length uuids.
> 
> Helper is now required for the new FS_IOC_GETUUID ioctl; if
> super_set_uuid() hasn't been called, the ioctl won't be supported.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/7] overlayfs: Convert to super_set_uuid()

[Dave Chinner]

On Tue, Feb 06, 2024 at 03:18:50PM -0500, Kent Overstreet wrote:
> We don't want to be settingc sb->s_uuid directly anymore, as there's a
> length field that also has to be set, and this conversion was not
> completely trivial.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: linux-unionfs@vger.kernel.org
> ---
>  fs/overlayfs/util.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 0217094c23ea..f1f0ee9a9dff 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
>  			 const struct path *upperpath)
>  {
>  	bool set = false;
> +	uuid_t uuid;
>  	int res;
>  
>  	/* Try to load existing persistent uuid */
> -	res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
> +	res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
>  				UUID_SIZE);
>  	if (res == UUID_SIZE)
> -		return true;
> +		goto success;
>  
>  	if (res != -ENODATA)
>  		goto fail;
> @@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
>  	}
>  
>  	/* Generate overlay instance uuid */
> -	uuid_gen(&sb->s_uuid);
> +	uuid_gen(&uuid);
>  
>  	/* Try to store persistent uuid */
>  	set = true;
> -	res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
> +	res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
>  			   UUID_SIZE);
>  	if (res == 0)
> -		return true;
> +		goto success;

This is a bit weird. Normally the success case is in line, and we
jump out of line for the fail case. I think this is more better:

	if (res)
		goto fail;
success:
	super_set_uuid(sb, uuid.b, sizeof(uuid));
	return true;
>  
>  fail:
>  	memset(sb->s_uuid.b, 0, UUID_SIZE);

And then the fail case follows naturally.

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Dave Chinner]

On Tue, Feb 06, 2024 at 03:18:51PM -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. We're not lifting SETFSUUID from ext4 -
> that can be done on offline filesystems by the people who need it,
> trying to do it online is just asking for too much trouble.
> 
> 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 | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 76cf22ac97d7..046c30294a82 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 (!sb->s_uuid_len)
> +		return -ENOIOCTLCMD;
> +
> +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> +
> +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> +}

Can we please keep the declarations separate from the code? I always
find this sort of implicit scoping of variables both difficult to
read (especially in larger functions) and a landmine waiting to be
tripped over. This could easily just be:

static int ioctl_getfsuuid(struct file *file, void __user *argp)
{
	struct super_block *sb = file_inode(file)->i_sb;
	struct fsuuid2 u = { .len = sb->s_uuid_len, };

	....

and then it's consistent with all the rest of the code...

> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 48ad69f7722e..16a6ecadfd8d 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -64,6 +64,19 @@ 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 {
> +	__u8	len;
> +	__u8	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
> @@ -190,6 +203,9 @@ struct fsxattr {
>   * (see uapi/linux/blkzoned.h)
>   */
>  
> +/* Returns the external filesystem UUID, the same one blkid returns */
> +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> +

Can you add a comment somewhere in the file saying that new VFS
ioctls should use the "0x12" namespace in the range 142-255, and
mention that BLK ioctls should be kept within the 0x12 {0-141}
range?

Probably also document this clearly in
Documentation/userspace-api/ioctl/ioctl-number.rst, too?

-Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 5/7] fs: FS_IOC_GETSYSFSNAME

[Dave Chinner]

On Tue, Feb 06, 2024 at 03:18:53PM -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).
> 
> 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>
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/ioctl.c              | 17 +++++++++++++++++
>  include/linux/fs.h      |  1 +
>  include/uapi/linux/fs.h | 10 ++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 046c30294a82..7c37765bd04e 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_get_fs_sysfs_path(struct file *file, void __user *argp)
> +{
> +	struct super_block *sb = file_inode(file)->i_sb;
> +
> +	if (!strlen(sb->s_sysfs_name))
> +		return -ENOIOCTLCMD;

This relies on the kernel developers getting string termination
right and not overflowing buffers.

We can do better, I think, and I suspect that starts with a
super_set_sysfs_name() helper....

> +	struct fssysfspath u = {};

Variable names that look like the cat just walked over the keyboard
are difficult to read. Please use some separators here! 

Also, same comment as previous about mixing code and declarations.

> +
> +	snprintf(u.name, sizeof(u.name), "%s/%s", sb->s_type->name, sb->s_sysfs_name);

What happens if the combined path overflows the fssysfspath
buffer?

> +
> +	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_GETFSSYSFSPATH:
> +		return ioctl_get_fs_sysfs_path(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 acdc56987cb1..b96c1d009718 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];

Can we just kstrdup() the sysfs name and keep a {ptr, len} pair
in the sb for this? Then we can treat them as an opaque identifier
that isn't actually a string, and we don't have to make up some
arbitrary maximum name size, either.

>  	unsigned int		s_max_links;
>  
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 16a6ecadfd8d..c0f7bd4b6350 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -77,6 +77,10 @@ struct fsuuid2 {
>  	__u8	uuid[16];
>  };
>  
> +struct fssysfspath {
> +	__u8			name[128];
> +};

Undocumented magic number in a UAPI. Why was this chosen?

IMO, we shouldn't be returning strings that require the the kernel
to place NULLs correctly and for the caller to detect said NULLs to
determine their length, so something like:

struct fs_sysfs_path {
	__u32			name_len;
	__u8			name[NAME_MAX];
};

Seems better to me...

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

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Kent Overstreet]

On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > +{
> > +	struct super_block *sb = file_inode(file)->i_sb;
> > +
> > +	if (!sb->s_uuid_len)
> > +		return -ENOIOCTLCMD;
> > +
> > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > +
> > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > +}
> 
> Can we please keep the declarations separate from the code? I always
> find this sort of implicit scoping of variables both difficult to
> read (especially in larger functions) and a landmine waiting to be
> tripped over. This could easily just be:
> 
> static int ioctl_getfsuuid(struct file *file, void __user *argp)
> {
> 	struct super_block *sb = file_inode(file)->i_sb;
> 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> 
> 	....
> 
> and then it's consistent with all the rest of the code...

The way I'm doing it here is actually what I'm transitioning my own code
to - the big reason being that always declaring variables at the tops of
functions leads to separating declaration and initialization, and worse
it leads people to declaring a variable once and reusing it for multiple
things (I've seen that be a source of real bugs too many times).

But I won't push that in this patch, we can just keep the style
consistent for now.

> > +/* Returns the external filesystem UUID, the same one blkid returns */
> > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > +
> 
> Can you add a comment somewhere in the file saying that new VFS
> ioctls should use the "0x12" namespace in the range 142-255, and
> mention that BLK ioctls should be kept within the 0x12 {0-141}
> range?

Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
separate ranges, then FS_IOC_ needs to move to something else becasue
otherwise BLK_ won't have a way to expand.

So what else -

ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
appears to be the only one without conflicts - all the other ranges seem
to have originated with other filesystems.

So perhaps I will take Darrick's nak (0x15) suggestion after all.

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Dave Chinner]

On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > +{
> > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > +
> > > +	if (!sb->s_uuid_len)
> > > +		return -ENOIOCTLCMD;
> > > +
> > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > +
> > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > +}
> > 
> > Can we please keep the declarations separate from the code? I always
> > find this sort of implicit scoping of variables both difficult to
> > read (especially in larger functions) and a landmine waiting to be
> > tripped over. This could easily just be:
> > 
> > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > {
> > 	struct super_block *sb = file_inode(file)->i_sb;
> > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > 
> > 	....
> > 
> > and then it's consistent with all the rest of the code...
> 
> The way I'm doing it here is actually what I'm transitioning my own code
> to - the big reason being that always declaring variables at the tops of
> functions leads to separating declaration and initialization, and worse
> it leads people to declaring a variable once and reusing it for multiple
> things (I've seen that be a source of real bugs too many times).
> 
> But I won't push that in this patch, we can just keep the style
> consistent for now.
> 
> > > +/* Returns the external filesystem UUID, the same one blkid returns */
> > > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > > +
> > 
> > Can you add a comment somewhere in the file saying that new VFS
> > ioctls should use the "0x12" namespace in the range 142-255, and
> > mention that BLK ioctls should be kept within the 0x12 {0-141}
> > range?
> 
> Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
> separate ranges, then FS_IOC_ needs to move to something else becasue
> otherwise BLK_ won't have a way to expand.

The BLK range can grow downwards towards zero, I think. It starts at
93 and goes to 136, so there's heaps of space for it to grow from 92
to 0....

> So what else -
> 
> ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
> appears to be the only one without conflicts - all the other ranges seem
> to have originated with other filesystems.

*nod*

> So perhaps I will take Darrick's nak (0x15) suggestion after all.

That works, too.

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

Re: [PATCH v2 5/7] fs: FS_IOC_GETSYSFSNAME

[Kent Overstreet]

On Wed, Feb 07, 2024 at 09:26:40AM +1100, Dave Chinner wrote:
> On Tue, Feb 06, 2024 at 03:18:53PM -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).
> > 
> > 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>
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > ---
> >  fs/ioctl.c              | 17 +++++++++++++++++
> >  include/linux/fs.h      |  1 +
> >  include/uapi/linux/fs.h | 10 ++++++++++
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 046c30294a82..7c37765bd04e 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_get_fs_sysfs_path(struct file *file, void __user *argp)
> > +{
> > +	struct super_block *sb = file_inode(file)->i_sb;
> > +
> > +	if (!strlen(sb->s_sysfs_name))
> > +		return -ENOIOCTLCMD;
> 
> This relies on the kernel developers getting string termination
> right and not overflowing buffers.
> 
> We can do better, I think, and I suspect that starts with a
> super_set_sysfs_name() helper....

I don't think that's needed here; a standard snprintf() ensures that the
buffer is null terminated, even if the result overflowed.

> > +	struct fssysfspath u = {};
> 
> Variable names that look like the cat just walked over the keyboard
> are difficult to read. Please use some separators here! 
> 
> Also, same comment as previous about mixing code and declarations.
> 
> > +
> > +	snprintf(u.name, sizeof(u.name), "%s/%s", sb->s_type->name, sb->s_sysfs_name);
> 
> What happens if the combined path overflows the fssysfspath
> buffer?

It won't; s_sysfs_name is going to be either a UUID or a short device
name. It can't be a longer device name (like we show in /proc/mounts) -
here that would lead to ambiguouty.

> > +	char			s_sysfs_name[UUID_STRING_LEN + 1];
> 
> Can we just kstrdup() the sysfs name and keep a {ptr, len} pair
> in the sb for this? Then we can treat them as an opaque identifier
> that isn't actually a string, and we don't have to make up some
> arbitrary maximum name size, either.

What if we went in a different direction - take your previous suggestion
to have a helper for setting the name, and then enforce through the API
that the only valid identifiers are a UUID or a short device name.

super_set_sysfs_identifier_uuid(sb);
super_set_sysfs_identifier_device(sb);

then we can enforce that the identifier comes from either sb->s_uuid or
sb->s_dev.

I'll have to check existing filesystems before we commit to that,
though.

> 
> >  	unsigned int		s_max_links;
> >  
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index 16a6ecadfd8d..c0f7bd4b6350 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -77,6 +77,10 @@ struct fsuuid2 {
> >  	__u8	uuid[16];
> >  };
> >  
> > +struct fssysfspath {
> > +	__u8			name[128];
> > +};
> 
> Undocumented magic number in a UAPI. Why was this chosen?

In this case, I think the magic number is fine - though it could use a
comment; since it only needs to be used in one place giving it a name is
a bit pointless.

As to why it was chosen - 64 is the next power of two size up from the
length of a uuid string length, and 64 should fit any UUID + filesystem
name. So took that and doubled it.

> IMO, we shouldn't be returning strings that require the the kernel
> to place NULLs correctly and for the caller to detect said NULLs to
> determine their length, so something like:
> 
> struct fs_sysfs_path {
> 	__u32			name_len;
> 	__u8			name[NAME_MAX];
> };
> 
> Seems better to me...

I suppose modern languages are getting away from the whole
nul-terminated string thing, heh

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Eric Biggers]

On Tue, Feb 06, 2024 at 03:18:48PM -0500, Kent Overstreet wrote:
> 
> Darrick also noticed that fscrypt (!) is using sb->s_uuid, which looks
> busted - they want to be using the "this can never change" UUID, but
> that is not an item for this patchset.
> 

fscrypt only uses sb->s_uuid if FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64 or
FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32 is being used in the encryption policy.
These flags are only supported by ext4 and f2fs, and they are only useful when
the file contents encryption is being done with inline encryption hardware that
only allows 64-bits or less of the initialization vector to be specified and
that has poor performance when switching keys.  This hardware is currently only
known to be present on mobile and embedded systems that use eMMC or UFS storage.

Note that these settings assume the inode numbers are stable as well as the
UUID.  So, when they are in use, filesystem shrinking is prohibited as well as
changing the filesystem UUID.  (In ext4, both operations are forbidden using the
stable_inodes feature flag.  f2fs doesn't support either operation regardless.)

These restrictions are unfortunate, but so far they haven't been a problem for
the only known use case for these non-default settings.

In the case of s_uuid, for both ext4 and f2fs it's true that we could have used
s_encrypt_pw_salt instead, or added a new general-purpose internal UUID field
and used that.  Maybe we even should have, considering the precedent of ext4's
metadata_csum migrating away from using the UUID to its own internal seed.  I do
worry that relying on an internal UUID for these settings would make it easier
for people to create insecure setups where they're using the same fscrypt key on
multiple filesystems with the same internal UUID.  With the external UUID, such
misconfigurations are obvious and will be noticed and fixed.  With the internal
UUID, such vulnerabilities would not be noticed, as things will "just work".
Which is better?  It's not entirely clear to me.  We do encourage the use of
different fscrypt keys on different filesystems anyway, but this isn't required.

Of course, even if the usability improvement outweighs that concern, switching
these already-existing encryption settings over to use an internal UUID can't be
done trivially; it would have to be controlled by a new filesystem feature flag.
We probably shouldn't bother unless/until there's a clear use case for it.

If anyone does have any new use case for these weird and non-default encryption
settings (and I hope you don't!), I'd be interested to hear...

- Eric

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Kent Overstreet]

On Tue, Feb 06, 2024 at 05:47:29PM -0800, Eric Biggers wrote:
> On Tue, Feb 06, 2024 at 03:18:48PM -0500, Kent Overstreet wrote:
> > 
> > Darrick also noticed that fscrypt (!) is using sb->s_uuid, which looks
> > busted - they want to be using the "this can never change" UUID, but
> > that is not an item for this patchset.
> > 
> 
> fscrypt only uses sb->s_uuid if FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64 or
> FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32 is being used in the encryption policy.
> These flags are only supported by ext4 and f2fs, and they are only useful when
> the file contents encryption is being done with inline encryption hardware that
> only allows 64-bits or less of the initialization vector to be specified and
> that has poor performance when switching keys.  This hardware is currently only
> known to be present on mobile and embedded systems that use eMMC or UFS storage.
> 
> Note that these settings assume the inode numbers are stable as well as the
> UUID.  So, when they are in use, filesystem shrinking is prohibited as well as
> changing the filesystem UUID.  (In ext4, both operations are forbidden using the
> stable_inodes feature flag.  f2fs doesn't support either operation regardless.)
> 
> These restrictions are unfortunate, but so far they haven't been a problem for
> the only known use case for these non-default settings.
> 
> In the case of s_uuid, for both ext4 and f2fs it's true that we could have used
> s_encrypt_pw_salt instead, or added a new general-purpose internal UUID field
> and used that.  Maybe we even should have, considering the precedent of ext4's
> metadata_csum migrating away from using the UUID to its own internal seed.  I do
> worry that relying on an internal UUID for these settings would make it easier
> for people to create insecure setups where they're using the same fscrypt key on
> multiple filesystems with the same internal UUID.  With the external UUID, such
> misconfigurations are obvious and will be noticed and fixed.  With the internal
> UUID, such vulnerabilities would not be noticed, as things will "just work".
> Which is better?  It's not entirely clear to me.  We do encourage the use of
> different fscrypt keys on different filesystems anyway, but this isn't required.

*nod* nonce reuse is a thorny issue - that makes using the changeable,
external UUID a bit more of a feature than a bug.

> Of course, even if the usability improvement outweighs that concern, switching
> these already-existing encryption settings over to use an internal UUID can't be
> done trivially; it would have to be controlled by a new filesystem feature flag.
> We probably shouldn't bother unless/until there's a clear use case for it.
> 
> If anyone does have any new use case for these weird and non-default encryption
> settings (and I hope you don't!), I'd be interested to hear...

I just wanted to make sure I wasn't breaking fscrypt by baking-in that
s_uuid is the user facing one - thanks for the info.

Can we get this documented in a code comment somewhere visible? It was
definitely a "wtf" moment when Darrick and I saw it, I want to make sure
people know what's going on later.

Re: [PATCH v2 2/7] overlayfs: Convert to super_set_uuid()

[Amir Goldstein]

On Tue, Feb 6, 2024 at 11:49 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Tue, Feb 06, 2024 at 03:18:50PM -0500, Kent Overstreet wrote:
> > We don't want to be settingc sb->s_uuid directly anymore, as there's a
> > length field that also has to be set, and this conversion was not
> > completely trivial.
> >
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: linux-unionfs@vger.kernel.org
> > ---
> >  fs/overlayfs/util.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 0217094c23ea..f1f0ee9a9dff 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> >                        const struct path *upperpath)
> >  {
> >       bool set = false;
> > +     uuid_t uuid;
> >       int res;
> >
> >       /* Try to load existing persistent uuid */
> > -     res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
> > +     res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
> >                               UUID_SIZE);
> >       if (res == UUID_SIZE)
> > -             return true;
> > +             goto success;
> >
> >       if (res != -ENODATA)
> >               goto fail;
> > @@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
> >       }
> >
> >       /* Generate overlay instance uuid */
> > -     uuid_gen(&sb->s_uuid);
> > +     uuid_gen(&uuid);
> >
> >       /* Try to store persistent uuid */
> >       set = true;
> > -     res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
> > +     res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
> >                          UUID_SIZE);
> >       if (res == 0)
> > -             return true;
> > +             goto success;
>
> This is a bit weird. Normally the success case is in line, and we
> jump out of line for the fail case. I think this is more better:
>
>         if (res)
>                 goto fail;
> success:
>         super_set_uuid(sb, uuid.b, sizeof(uuid));
>         return true;
> >
> >  fail:
> >       memset(sb->s_uuid.b, 0, UUID_SIZE);
>
> And then the fail case follows naturally.
>

I agree. Please use the label name
set_uuid:

Also the memset() in fail: case is not needed anymore, because
with your change we do not dirty sb->s_uuid in this function.

Thanks,
Amir.

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Brian Foster]

On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > +{
> > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > +
> > > +	if (!sb->s_uuid_len)
> > > +		return -ENOIOCTLCMD;
> > > +
> > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > +
> > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > +}
> > 
> > Can we please keep the declarations separate from the code? I always
> > find this sort of implicit scoping of variables both difficult to
> > read (especially in larger functions) and a landmine waiting to be
> > tripped over. This could easily just be:
> > 
> > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > {
> > 	struct super_block *sb = file_inode(file)->i_sb;
> > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > 
> > 	....
> > 
> > and then it's consistent with all the rest of the code...
> 
> The way I'm doing it here is actually what I'm transitioning my own code
> to - the big reason being that always declaring variables at the tops of
> functions leads to separating declaration and initialization, and worse
> it leads people to declaring a variable once and reusing it for multiple
> things (I've seen that be a source of real bugs too many times).
> 

I still think this is of questionable value. I know I've mentioned
similar concerns to Dave's here on the bcachefs list, but still have not
really seen any discussion other than a bit of back and forth on the
handful of generally accepted (in the kernel) uses of this sort of thing
for limiting scope in loops/branches and such.

I was skimming through some more recent bcachefs patches the other day
(the journal write pipelining stuff) where I came across one or two
medium length functions where this had proliferated, and I found it kind
of annoying TBH. It starts to almost look like there are casts all over
the place and it's a bit more tedious to filter out logic from the
additional/gratuitous syntax, IMO.

That's still just my .02, but there was also previous mention of
starting/having discussion on this sort of style change. Is that still
the plan? If so, before or after proliferating it throughout the
bcachefs code? ;) I am curious if there are other folks in kernel land
who think this makes enough sense that they'd plan to adopt it. Hm?

Brian

> But I won't push that in this patch, we can just keep the style
> consistent for now.
> 
> > > +/* Returns the external filesystem UUID, the same one blkid returns */
> > > +#define FS_IOC_GETFSUUID		_IOR(0x12, 142, struct fsuuid2)
> > > +
> > 
> > Can you add a comment somewhere in the file saying that new VFS
> > ioctls should use the "0x12" namespace in the range 142-255, and
> > mention that BLK ioctls should be kept within the 0x12 {0-141}
> > range?
> 
> Well, if we're going to try to keep the BLK_ and FS_IOC_ ioctls in
> separate ranges, then FS_IOC_ needs to move to something else becasue
> otherwise BLK_ won't have a way to expand.
> 
> So what else -
> 
> ioctl-number.rst has a bunch of other ranges listed for fs.h, but 0x12
> appears to be the only one without conflicts - all the other ranges seem
> to have originated with other filesystems.
> 
> So perhaps I will take Darrick's nak (0x15) suggestion after all.
> 

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Theodore Ts'o]

On Tue, Feb 06, 2024 at 03:18:48PM -0500, Kent Overstreet wrote:
> previous:
> https://lore.kernel.org/linux-fsdevel/20240206-aufwuchs-atomkraftgegner-dc53ce1e435f@brauner/T/
> 
> Changes since v1:
>  - super_set_uuid() helper, per Dave
> 
>  - nix FS_IOC_SETUUID - Al raised this and I'm in 100% agreement,
>    changing a UUID on an existing filesystem is a rare operation that
>    should only be done when the filesystem is offline; we'd need to
>    audit/fix a bunch of stuff if we wanted to support this

NAK.  First, this happens every single time a VM in the cloud starts
up, so it happens ZILLIONS of time a day.  Secondly, there is already
userspace programs --- to wit, tune2fs --- that uses this ioctl, so
nixing FS_IOC_SETUUID will break userspace programs.

							- Ted

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Kent Overstreet]

On Wed, Feb 07, 2024 at 12:40:09PM -0500, Theodore Ts'o wrote:
> On Tue, Feb 06, 2024 at 03:18:48PM -0500, Kent Overstreet wrote:
> > previous:
> > https://lore.kernel.org/linux-fsdevel/20240206-aufwuchs-atomkraftgegner-dc53ce1e435f@brauner/T/
> > 
> > Changes since v1:
> >  - super_set_uuid() helper, per Dave
> > 
> >  - nix FS_IOC_SETUUID - Al raised this and I'm in 100% agreement,
> >    changing a UUID on an existing filesystem is a rare operation that
> >    should only be done when the filesystem is offline; we'd need to
> >    audit/fix a bunch of stuff if we wanted to support this
> 
> NAK.  First, this happens every single time a VM in the cloud starts
> up, so it happens ZILLIONS of time a day.  Secondly, there is already
> userspace programs --- to wit, tune2fs --- that uses this ioctl, so
> nixing FS_IOC_SETUUID will break userspace programs.

You've still got the ext4 version, we're not taking that away. But I
don't think other filesystems will want to deal with the hassle of
changing UUIDs at runtime, since that's effectively used for API access
via sysfs and debugfs.

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Christian Brauner]

> don't think other filesystems will want to deal with the hassle of
> changing UUIDs at runtime, since that's effectively used for API access
> via sysfs and debugfs.

/me nods.

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Christian Brauner]

> Christain, if nothing else comes up, are you ready to take this?

I'm amazed how consistently you mistype my name. Sorry, I just read
that. Yep, I'm about to pick this up.

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Kent Overstreet]

On Thu, Feb 08, 2024 at 10:48:31AM +0100, Christian Brauner wrote:
> > Christain, if nothing else comes up, are you ready to take this?
> 
> I'm amazed how consistently you mistype my name. Sorry, I just read
> that. Yep, I'm about to pick this up.

Gah, am I becoming dyslexic in my old age...

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Kent Overstreet]

On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote:
> On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > +{
> > > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > > +
> > > > +	if (!sb->s_uuid_len)
> > > > +		return -ENOIOCTLCMD;
> > > > +
> > > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > > +
> > > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > > +}
> > > 
> > > Can we please keep the declarations separate from the code? I always
> > > find this sort of implicit scoping of variables both difficult to
> > > read (especially in larger functions) and a landmine waiting to be
> > > tripped over. This could easily just be:
> > > 
> > > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > {
> > > 	struct super_block *sb = file_inode(file)->i_sb;
> > > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > 
> > > 	....
> > > 
> > > and then it's consistent with all the rest of the code...
> > 
> > The way I'm doing it here is actually what I'm transitioning my own code
> > to - the big reason being that always declaring variables at the tops of
> > functions leads to separating declaration and initialization, and worse
> > it leads people to declaring a variable once and reusing it for multiple
> > things (I've seen that be a source of real bugs too many times).
> > 
> 
> I still think this is of questionable value. I know I've mentioned
> similar concerns to Dave's here on the bcachefs list, but still have not
> really seen any discussion other than a bit of back and forth on the
> handful of generally accepted (in the kernel) uses of this sort of thing
> for limiting scope in loops/branches and such.
> 
> I was skimming through some more recent bcachefs patches the other day
> (the journal write pipelining stuff) where I came across one or two
> medium length functions where this had proliferated, and I found it kind
> of annoying TBH. It starts to almost look like there are casts all over
> the place and it's a bit more tedious to filter out logic from the
> additional/gratuitous syntax, IMO.
> 
> That's still just my .02, but there was also previous mention of
> starting/having discussion on this sort of style change. Is that still
> the plan? If so, before or after proliferating it throughout the
> bcachefs code? ;) I am curious if there are other folks in kernel land
> who think this makes enough sense that they'd plan to adopt it. Hm?

That was the discussion :)

bcachefs is my codebase, so yes, I intend to do it there. I really think
this is an instance where you and Dave are used to the way C has
historically forced us to do things; our brains get wired to read code a
certain way and changes are jarring.

But take a step back; if we were used to writing code the way I'm doing
it, and you were arguing for putting declarations at the tops of
functions, what would the arguments be?

I would say you're just breaking up the flow of ideas for no reason; a
chain of related statements now includes a declaration that isn't with
the actual logic.

And bugs due to variable reuse, missed initialization - there's real
reasons not to do it that way.

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Brian Foster]

On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote:
> On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote:
> > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > > +{
> > > > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > > > +
> > > > > +	if (!sb->s_uuid_len)
> > > > > +		return -ENOIOCTLCMD;
> > > > > +
> > > > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > > > +
> > > > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > > > +}
> > > > 
> > > > Can we please keep the declarations separate from the code? I always
> > > > find this sort of implicit scoping of variables both difficult to
> > > > read (especially in larger functions) and a landmine waiting to be
> > > > tripped over. This could easily just be:
> > > > 
> > > > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > {
> > > > 	struct super_block *sb = file_inode(file)->i_sb;
> > > > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > 
> > > > 	....
> > > > 
> > > > and then it's consistent with all the rest of the code...
> > > 
> > > The way I'm doing it here is actually what I'm transitioning my own code
> > > to - the big reason being that always declaring variables at the tops of
> > > functions leads to separating declaration and initialization, and worse
> > > it leads people to declaring a variable once and reusing it for multiple
> > > things (I've seen that be a source of real bugs too many times).
> > > 
> > 
> > I still think this is of questionable value. I know I've mentioned
> > similar concerns to Dave's here on the bcachefs list, but still have not
> > really seen any discussion other than a bit of back and forth on the
> > handful of generally accepted (in the kernel) uses of this sort of thing
> > for limiting scope in loops/branches and such.
> > 
> > I was skimming through some more recent bcachefs patches the other day
> > (the journal write pipelining stuff) where I came across one or two
> > medium length functions where this had proliferated, and I found it kind
> > of annoying TBH. It starts to almost look like there are casts all over
> > the place and it's a bit more tedious to filter out logic from the
> > additional/gratuitous syntax, IMO.
> > 
> > That's still just my .02, but there was also previous mention of
> > starting/having discussion on this sort of style change. Is that still
> > the plan? If so, before or after proliferating it throughout the
> > bcachefs code? ;) I am curious if there are other folks in kernel land
> > who think this makes enough sense that they'd plan to adopt it. Hm?
> 
> That was the discussion :)
> 
> bcachefs is my codebase, so yes, I intend to do it there. I really think
> this is an instance where you and Dave are used to the way C has
> historically forced us to do things; our brains get wired to read code a
> certain way and changes are jarring.
> 

Heh, fair enough. That's certainly your prerogative. I'm certainly not
trying to tell you what to do or not with bcachefs. That's at least
direct enough that it's clear it's not worth debating too much. ;)

> But take a step back; if we were used to writing code the way I'm doing
> it, and you were arguing for putting declarations at the tops of
> functions, what would the arguments be?
> 

I think my thought process would be similar. I.e., is the proposed
benefit of such a change worth the tradeoffs?

> I would say you're just breaking up the flow of ideas for no reason; a
> chain of related statements now includes a declaration that isn't with
> the actual logic.
> 
> And bugs due to variable reuse, missed initialization - there's real
> reasons not to do it that way.
> 

And were I in that position, I don't think I would reduce a decision
that affects readability/reviewability of my subsystem to a nontrivial
degree (for other people, at least) to that single aspect. This would be
the answer to the question: "is this worth considering?"

Brian

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Kent Overstreet]

On Mon, Feb 12, 2024 at 07:47:00AM -0500, Brian Foster wrote:
> On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote:
> > On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote:
> > > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> > > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > > > +{
> > > > > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > > > > +
> > > > > > +	if (!sb->s_uuid_len)
> > > > > > +		return -ENOIOCTLCMD;
> > > > > > +
> > > > > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > > > > +
> > > > > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > > > > +}
> > > > > 
> > > > > Can we please keep the declarations separate from the code? I always
> > > > > find this sort of implicit scoping of variables both difficult to
> > > > > read (especially in larger functions) and a landmine waiting to be
> > > > > tripped over. This could easily just be:
> > > > > 
> > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > > {
> > > > > 	struct super_block *sb = file_inode(file)->i_sb;
> > > > > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > > 
> > > > > 	....
> > > > > 
> > > > > and then it's consistent with all the rest of the code...
> > > > 
> > > > The way I'm doing it here is actually what I'm transitioning my own code
> > > > to - the big reason being that always declaring variables at the tops of
> > > > functions leads to separating declaration and initialization, and worse
> > > > it leads people to declaring a variable once and reusing it for multiple
> > > > things (I've seen that be a source of real bugs too many times).
> > > > 
> > > 
> > > I still think this is of questionable value. I know I've mentioned
> > > similar concerns to Dave's here on the bcachefs list, but still have not
> > > really seen any discussion other than a bit of back and forth on the
> > > handful of generally accepted (in the kernel) uses of this sort of thing
> > > for limiting scope in loops/branches and such.
> > > 
> > > I was skimming through some more recent bcachefs patches the other day
> > > (the journal write pipelining stuff) where I came across one or two
> > > medium length functions where this had proliferated, and I found it kind
> > > of annoying TBH. It starts to almost look like there are casts all over
> > > the place and it's a bit more tedious to filter out logic from the
> > > additional/gratuitous syntax, IMO.
> > > 
> > > That's still just my .02, but there was also previous mention of
> > > starting/having discussion on this sort of style change. Is that still
> > > the plan? If so, before or after proliferating it throughout the
> > > bcachefs code? ;) I am curious if there are other folks in kernel land
> > > who think this makes enough sense that they'd plan to adopt it. Hm?
> > 
> > That was the discussion :)
> > 
> > bcachefs is my codebase, so yes, I intend to do it there. I really think
> > this is an instance where you and Dave are used to the way C has
> > historically forced us to do things; our brains get wired to read code a
> > certain way and changes are jarring.
> > 
> 
> Heh, fair enough. That's certainly your prerogative. I'm certainly not
> trying to tell you what to do or not with bcachefs. That's at least
> direct enough that it's clear it's not worth debating too much. ;)
> 
> > But take a step back; if we were used to writing code the way I'm doing
> > it, and you were arguing for putting declarations at the tops of
> > functions, what would the arguments be?
> > 
> 
> I think my thought process would be similar. I.e., is the proposed
> benefit of such a change worth the tradeoffs?
> 
> > I would say you're just breaking up the flow of ideas for no reason; a
> > chain of related statements now includes a declaration that isn't with
> > the actual logic.
> > 
> > And bugs due to variable reuse, missed initialization - there's real
> > reasons not to do it that way.
> > 
> 
> And were I in that position, I don't think I would reduce a decision
> that affects readability/reviewability of my subsystem to a nontrivial
> degree (for other people, at least) to that single aspect. This would be
> the answer to the question: "is this worth considering?"

If you feel this affected by this, how are you going to cope with Rust?

Re: [PATCH v2 3/7] fs: FS_IOC_GETUUID

[Brian Foster]

On Mon, Feb 12, 2024 at 08:39:29AM -0500, Kent Overstreet wrote:
> On Mon, Feb 12, 2024 at 07:47:00AM -0500, Brian Foster wrote:
> > On Thu, Feb 08, 2024 at 04:57:02PM -0500, Kent Overstreet wrote:
> > > On Wed, Feb 07, 2024 at 08:05:29AM -0500, Brian Foster wrote:
> > > > On Tue, Feb 06, 2024 at 05:37:22PM -0500, Kent Overstreet wrote:
> > > > > On Wed, Feb 07, 2024 at 09:01:05AM +1100, Dave Chinner wrote:
> > > > > > On Tue, Feb 06, 2024 at 03:18:51PM -0500, Kent Overstreet wrote:
> > > > > > > +static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > > > > +{
> > > > > > > +	struct super_block *sb = file_inode(file)->i_sb;
> > > > > > > +
> > > > > > > +	if (!sb->s_uuid_len)
> > > > > > > +		return -ENOIOCTLCMD;
> > > > > > > +
> > > > > > > +	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > > > > +	memcpy(&u.uuid[0], &sb->s_uuid, sb->s_uuid_len);
> > > > > > > +
> > > > > > > +	return copy_to_user(argp, &u, sizeof(u)) ? -EFAULT : 0;
> > > > > > > +}
> > > > > > 
> > > > > > Can we please keep the declarations separate from the code? I always
> > > > > > find this sort of implicit scoping of variables both difficult to
> > > > > > read (especially in larger functions) and a landmine waiting to be
> > > > > > tripped over. This could easily just be:
> > > > > > 
> > > > > > static int ioctl_getfsuuid(struct file *file, void __user *argp)
> > > > > > {
> > > > > > 	struct super_block *sb = file_inode(file)->i_sb;
> > > > > > 	struct fsuuid2 u = { .len = sb->s_uuid_len, };
> > > > > > 
> > > > > > 	....
> > > > > > 
> > > > > > and then it's consistent with all the rest of the code...
> > > > > 
> > > > > The way I'm doing it here is actually what I'm transitioning my own code
> > > > > to - the big reason being that always declaring variables at the tops of
> > > > > functions leads to separating declaration and initialization, and worse
> > > > > it leads people to declaring a variable once and reusing it for multiple
> > > > > things (I've seen that be a source of real bugs too many times).
> > > > > 
> > > > 
> > > > I still think this is of questionable value. I know I've mentioned
> > > > similar concerns to Dave's here on the bcachefs list, but still have not
> > > > really seen any discussion other than a bit of back and forth on the
> > > > handful of generally accepted (in the kernel) uses of this sort of thing
> > > > for limiting scope in loops/branches and such.
> > > > 
> > > > I was skimming through some more recent bcachefs patches the other day
> > > > (the journal write pipelining stuff) where I came across one or two
> > > > medium length functions where this had proliferated, and I found it kind
> > > > of annoying TBH. It starts to almost look like there are casts all over
> > > > the place and it's a bit more tedious to filter out logic from the
> > > > additional/gratuitous syntax, IMO.
> > > > 
> > > > That's still just my .02, but there was also previous mention of
> > > > starting/having discussion on this sort of style change. Is that still
> > > > the plan? If so, before or after proliferating it throughout the
> > > > bcachefs code? ;) I am curious if there are other folks in kernel land
> > > > who think this makes enough sense that they'd plan to adopt it. Hm?
> > > 
> > > That was the discussion :)
> > > 
> > > bcachefs is my codebase, so yes, I intend to do it there. I really think
> > > this is an instance where you and Dave are used to the way C has
> > > historically forced us to do things; our brains get wired to read code a
> > > certain way and changes are jarring.
> > > 
> > 
> > Heh, fair enough. That's certainly your prerogative. I'm certainly not
> > trying to tell you what to do or not with bcachefs. That's at least
> > direct enough that it's clear it's not worth debating too much. ;)
> > 
> > > But take a step back; if we were used to writing code the way I'm doing
> > > it, and you were arguing for putting declarations at the tops of
> > > functions, what would the arguments be?
> > > 
> > 
> > I think my thought process would be similar. I.e., is the proposed
> > benefit of such a change worth the tradeoffs?
> > 
> > > I would say you're just breaking up the flow of ideas for no reason; a
> > > chain of related statements now includes a declaration that isn't with
> > > the actual logic.
> > > 
> > > And bugs due to variable reuse, missed initialization - there's real
> > > reasons not to do it that way.
> > > 
> > 
> > And were I in that position, I don't think I would reduce a decision
> > that affects readability/reviewability of my subsystem to a nontrivial
> > degree (for other people, at least) to that single aspect. This would be
> > the answer to the question: "is this worth considering?"
> 
> If you feel this affected by this, how are you going to cope with Rust?
> 

Well I'm still a Rust newbie, but I've been exposed to some of the basic
syntax and semantics and it hasn't been a problem yet. I'll keep my
fingers crossed, I guess.

Brian

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Theodore Ts'o]

On Wed, Feb 07, 2024 at 03:26:55PM -0500, Kent Overstreet wrote:
> You've still got the ext4 version, we're not taking that away. But I
> don't think other filesystems will want to deal with the hassle of
> changing UUIDs at runtime, since that's effectively used for API access
> via sysfs and debugfs.

Thanks. I misunderstood the log.  I didn't realize this was just about
not hoisting the ioctl to the VFS level, and dropping the generic uuid
set.

I'm not convinced that we should be using the UUID for kernel API
access, if for no other reason that not all file systems have UUID's.
Sure, modern file systems have UUID's, and individual file systems
might have to have specific features that don't play well with UUID's
changing while the file system is mounted.  But I'm hoping that we
don't add any new interfaces that rely on using the UUID for API
access at the VFS layer.  After all, ext2 (not just ext3 and ext4) has
supported changing the UUID while the file system has been mounted for
*decades*.

     	       	    	       	    	    - Ted

Re: [PATCH v2 0/7] filesystem visibililty ioctls

[Kent Overstreet]

On Mon, Feb 12, 2024 at 05:47:40PM -0500, Theodore Ts'o wrote:
> On Wed, Feb 07, 2024 at 03:26:55PM -0500, Kent Overstreet wrote:
> > You've still got the ext4 version, we're not taking that away. But I
> > don't think other filesystems will want to deal with the hassle of
> > changing UUIDs at runtime, since that's effectively used for API access
> > via sysfs and debugfs.
> 
> Thanks. I misunderstood the log.  I didn't realize this was just about
> not hoisting the ioctl to the VFS level, and dropping the generic uuid
> set.
> 
> I'm not convinced that we should be using the UUID for kernel API
> access, if for no other reason that not all file systems have UUID's.
> Sure, modern file systems have UUID's, and individual file systems
> might have to have specific features that don't play well with UUID's
> changing while the file system is mounted.  But I'm hoping that we
> don't add any new interfaces that rely on using the UUID for API
> access at the VFS layer.  After all, ext2 (not just ext3 and ext4) has
> supported changing the UUID while the file system has been mounted for
> *decades*.

*nod*

The intention isn't for every filesystem to be using the UUID for API
access - there's no reason to for single device filesystems, after all.

The intent is rather - for filesystems that _do_ need the UUID as a
stable identifier, let's try to standardize how's it's exposed and
used...