On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang
<catherine.hoang@xxxxxxxxxx> wrote:
>
> > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
> > <catherine.hoang@xxxxxxxxxx> wrote:
> >>
> >> Add a new ioctl to set the uuid of a mounted filesystem.
> >>
> >> Signed-off-by: Catherine Hoang <catherine.hoang@xxxxxxxxxx>
> >> ---
> >> fs/xfs/libxfs/xfs_fs.h | 1 +
> >> fs/xfs/xfs_ioctl.c | 107 +++++++++++++++++++++++++++++++++++++++++
> >> fs/xfs/xfs_log.c | 19 ++++++++
> >> fs/xfs/xfs_log.h | 2 +
> >> 4 files changed, 129 insertions(+)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> >> index 1cfd5bc6520a..a350966cce99 100644
> >> --- a/fs/xfs/libxfs/xfs_fs.h
> >> +++ b/fs/xfs/libxfs/xfs_fs.h
> >> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
> >> #define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom)
> >> #define XFS_IOC_BULKSTAT _IOR ('X', 127, struct xfs_bulkstat_req)
> >> #define XFS_IOC_INUMBERS _IOR ('X', 128, struct xfs_inumbers_req)
> >> +#define XFS_IOC_SETFSUUID _IOR ('X', 129, uuid_t)
> >
> > Should be _IOW.
>
> Ok, will fix that.
> >
> > Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
> > so that other fs could implement it later on, instead of hoisting it later?
> >
> > It would be easy to add support for FS_IOC_SETFSUUID to ext4
> > by generalizing ext4_ioctl_setuuid().
> >
> > Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
> > to fs.h and use that ioctl also for xfs.
>
> I actually did try to hoist the ext4 ioctls previously, but we weren’t able to come
> to a consensus on the implementation.
>
> https://lore.kernel.org/linux-xfs/20221118211408.72796-2-catherine.hoang@xxxxxxxxxx/
>
> I would prefer to keep this defined as an xfs specific ioctl to avoid all of the
> fsdevel bikeshedding.
For the greater good, please do try to have this bikeshedding, before giving up.
The discussion you pointed to wasn't so far from consensus IMO except
fsdevel was not CCed.
> >
> > Using an extensible struct with flags for that ioctl may turn out to be useful,
> > for example, to verify that the new uuid is unique, despite the fact
> > that xfs was
> > mounted with -onouuid (useful IMO) or to explicitly request a restore of old
> > uuid that would fail if new_uuid != meta uuid.
>
> I think using a struct is probably a good idea, I can add that in the next version.
Well, if you agree about a struct and agree about flags then the only thing
left is Dave's concern about variable size arrays in ioctl and that could be
addressed in a way that is compatible with ext4.
See untested patch below.
Thanks,
Amir.
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b7b56871029c..143a4735486e 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -215,6 +215,17 @@ 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('v', 44, struct fsuuid)
+#define FS_IOC_SETFSUUID _IOW('v', 44, struct fsuuid)
+
+/*
+ * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID
+ */
+struct fsuuid {
+ __u32 fsu_len; /* for backward compat has to be 16 */
+ __u32 fsu_flags;
+ __u8 fsu_uuid[16];
+};
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 140e1eb300d1..c4ded5d5e421 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -722,8 +722,8 @@ enum {
#define EXT4_IOC_GETSTATE _IOW('f', 41, __u32)
#define EXT4_IOC_GET_ES_CACHE _IOWR('f', 42, struct fiemap)
#define EXT4_IOC_CHECKPOINT _IOW('f', 43, __u32)
-#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid)
-#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid)
+#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid_bdr)
+#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid_hdr)
#define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
@@ -756,7 +756,7 @@ enum {
/*
* Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
*/
-struct fsuuid {
+struct fsuuid_hdr {
__u32 fsu_len;
__u32 fsu_flags;
__u8 fsu_uuid[];
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 8067ccda34e4..fc744231ad24 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1149,7 +1149,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
struct fsuuid fsuuid;
__u8 uuid[UUID_SIZE];
- if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
+ if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid)))
return -EFAULT;
if (fsuuid.fsu_len == 0) {
@@ -1168,7 +1168,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
unlock_buffer(sbi->s_sbh);
fsuuid.fsu_len = UUID_SIZE;
- if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
+ if (copy_to_user(ufsuuid, &fsuuid, offsetof(fsuuid, fsu_uuid)) ||
copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
return -EFAULT;
return 0;
@@ -1194,7 +1194,7 @@ static int ext4_ioctl_setuuid(struct file *filp,
|| ext4_has_feature_stable_inodes(sb))
return -EOPNOTSUPP;
- if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
+ if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid)))
return -EFAULT;
if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
@@ -1596,8 +1596,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);
default:
