| Message ID | 20220719065551.154132-1-bongiojp@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Add ioctls to get/set the ext4 superblock uuid. | expand |
On Tue, Jul 19, 2022 at 8:55 AM Jeremy Bongio <bongiojp@gmail.com> wrote: > > This fixes a race between changing the ext4 superblock uuid and operations > like mounting, resizing, changing features, etc. > > Reviewed-by: Theodore Ts'o <tytso@mit.edu> > Signed-off-by: Jeremy Bongio <bongiojp@gmail.com> > --- > > This pair of ioctls may be implemented in more filesystems in the future, > namely XFS. > > +++ b/fs/ext4/ext4.h > @@ -724,6 +724,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) The implementation looks good to me, but maybe it should be defined in the UAPI headers in a filesystem-independent way? Having it in a private header means it will not be available to portable user programs, and will be hidden from tools like strace that parse the uapi headers to find ioctl definitions. Arnd
Hi Jeremy,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master v5.19-rc7 next-20220718]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jeremy-Bongio/Add-ioctls-to-get-set-the-ext4-superblock-uuid/20220719-145724
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-defconfig (https://download.01.org/0day-ci/archive/20220719/202207191835.R8aDmooK-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/d53b0a271606a7f5c7b0f1a1f3fec9a34e771205
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jeremy-Bongio/Add-ioctls-to-get-set-the-ext4-superblock-uuid/20220719-145724
git checkout d53b0a271606a7f5c7b0f1a1f3fec9a34e771205
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
fs/ext4/ioctl.c: In function 'ext4_ioctl_getuuid':
>> fs/ext4/ioctl.c:1149:13: warning: unused variable 'ret' [-Wunused-variable]
1149 | int ret = 0;
| ^~~
vim +/ret +1149 fs/ext4/ioctl.c
1145
1146 static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
1147 struct fsuuid __user *ufsuuid)
1148 {
> 1149 int ret = 0;
1150 struct fsuuid fsuuid;
1151 __u8 uuid[UUID_SIZE];
1152
1153 if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
1154 return -EFAULT;
1155
1156 if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
1157 return -EINVAL;
1158
1159 lock_buffer(sbi->s_sbh);
1160 memcpy(uuid, sbi->s_es->s_uuid, UUID_SIZE);
1161 unlock_buffer(sbi->s_sbh);
1162
1163 if (copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
1164 return -EFAULT;
1165 return 0;
1166 }
1167
On Tue, Jul 19, 2022 at 09:30:23AM +0200, Arnd Bergmann wrote: > On Tue, Jul 19, 2022 at 8:55 AM Jeremy Bongio <bongiojp@gmail.com> wrote: > > This pair of ioctls may be implemented in more filesystems in the future, > > namely XFS. > > > > > +++ b/fs/ext4/ext4.h > > @@ -724,6 +724,8 @@ enum { > > +#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid) > > +#define EXT4_IOC_SETFSUUID _IOW('f', 44, struct fsuuid) > > The implementation looks good to me, but maybe it should be defined in > the UAPI headers in a filesystem-independent way? Having it in a private > header means it will not be available to portable user programs, and will > be hidden from tools like strace that parse the uapi headers to find > ioctl definitions. The plan is that when another file system implements it, we'll promote this to be a include/uapi/linux/fs.h. For e2fsprogs, we've always hard-coded the ioctl definitions as a default, because we don't want to be tied to having the correct version for the kernel header files --- I consider it important that there aren't variences in what kind of functionality you get if you build e2fsprogs on RHEL vs Fedora vs Debian Stable. And at least initially, the primary consumer of the ioctl will be tune2fs. As to why we define things first in the file system header, part of this is due to some interesting dynamics around bike-shedding. It is perceived that there are times when the bike-shed brigade comes up in full force, giving conflicting demands, and generally preventing forward progress, and so one of the reasons why file system developers often want to define things first a file-system dependent way is as a safety value so that we can blow past "unreasonable" demands. This recently came up in a LSF/MM discussion where Kent Overstreet proposed a new "ioctl" mechanism which promised that all ioctls would go through the full strict syscall ABI review and that there be no way to bypass it --- and in his view, this was considered a feature, and not a bug. Interestingly, after this LSF/MM discussion, a certain major file system maintainer (not myself) stated in a hallway coversation that due to unreasonable bike-sheeding, he was planning on bypassing the whole review process and just defining in an fs-dependent ioctl in an fs-specific header file because he was so f***** frustrated by the process. Of course, for every unreasonable bike-shedding, there are cases like the dedup ioctl which has recently observed that the interface was terrible, and it *should* have gone through more careful review before making it be a user-visible interface that multiple file systems now have to deal with. At this point, my personal "Via Media" approach to this is to send the patches for full review to all of the usual places, so we *can* get the benefits of interfave review. However, if things go pear-shaped, since the ioctl's are defined as fs-specific I can pull the "I'm the XXX fs maintainer" card, and just include it in my next pull request to Linus. If ioctl has a reasonable interface, then other file system maintainers can choose to adopt it, at which point we promote it to be an fs-independent ioctl. Or maybe they'll define their own fs-depedent ioctl, and we iterate in that way, using a market-forces dynamics ala how we have independent Linux distributions which compete with one another to provide a better use experience, as opposed to a single One True Userspace under the authority of the NetBSD or FreeBSD core team. It's not a perfect mechanism, but given that we don't have something like an Architectural Review Board with appeals up to some management chain if said ARB becomes obstructive (which is how things might work in a corporate environment), it's the best approach I've been able to come up with. - Ted P.S. BTW, this isn't a problem which is unique to system calls, but new file system icotls seem to be defined much more often than system calls. Whether that's because we naturally need many more of these interfaces, many of which are used by primarily by the XXX-fsprogs utilities, or because it's an escape hatch when the system call review process is perceived, correctly, or incorrectly, in too heavy-weight and prone to bike-shedding, is certainly a debatable point. But perhaps this is more of a Maintainer's Summit topic, although I don't think there really is a good solution other than "sometimes, someone in a position of power, whether it's Linus or a fs maintainer, has to be able to use an escape hatch when the process goes sideways.
Hi Jeremy,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master v5.19-rc7 next-20220719]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jeremy-Bongio/Add-ioctls-to-get-set-the-ext4-superblock-uuid/20220719-145724
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-randconfig-a005-20220718 (https://download.01.org/0day-ci/archive/20220720/202207200113.9R6VYmdT-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fa0c7639e91fa1cd0cf2ff0445a1634a90fe850a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d53b0a271606a7f5c7b0f1a1f3fec9a34e771205
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jeremy-Bongio/Add-ioctls-to-get-set-the-ext4-superblock-uuid/20220719-145724
git checkout d53b0a271606a7f5c7b0f1a1f3fec9a34e771205
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> fs/ext4/ioctl.c:1149:6: warning: unused variable 'ret' [-Wunused-variable]
int ret = 0;
^
1 warning generated.
vim +/ret +1149 fs/ext4/ioctl.c
1145
1146 static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
1147 struct fsuuid __user *ufsuuid)
1148 {
> 1149 int ret = 0;
1150 struct fsuuid fsuuid;
1151 __u8 uuid[UUID_SIZE];
1152
1153 if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
1154 return -EFAULT;
1155
1156 if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
1157 return -EINVAL;
1158
1159 lock_buffer(sbi->s_sbh);
1160 memcpy(uuid, sbi->s_es->s_uuid, UUID_SIZE);
1161 unlock_buffer(sbi->s_sbh);
1162
1163 if (copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
1164 return -EFAULT;
1165 return 0;
1166 }
1167
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 75b8d81b2469..b760d669a1ca 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -724,6 +724,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_SHUTDOWN _IOR ('X', 125, __u32) @@ -753,6 +755,15 @@ enum { EXT4_IOC_CHECKPOINT_FLAG_ZEROOUT | \ EXT4_IOC_CHECKPOINT_FLAG_DRY_RUN) +/* + * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID + */ +struct fsuuid { + __u32 fsu_len; + __u32 fsu_flags; + __u8 fsu_uuid[]; +}; + #if defined(__KERNEL__) && defined(CONFIG_COMPAT) /* * ioctl commands in 32 bit emulation diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index cb01c1da0f9d..59d320719596 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -20,6 +20,7 @@ #include <linux/delay.h> #include <linux/iversion.h> #include <linux/fileattr.h> +#include <linux/uuid.h> #include "ext4_jbd2.h" #include "ext4.h" #include <linux/fsmap.h> @@ -41,6 +42,15 @@ static void ext4_sb_setlabel(struct ext4_super_block *es, const void *arg) memcpy(es->s_volume_name, (char *)arg, EXT4_LABEL_MAX); } +/* + * Superblock modification callback function for changing file system + * UUID. + */ +static void ext4_sb_setuuid(struct ext4_super_block *es, const void *arg) +{ + memcpy(es->s_uuid, (__u8 *)arg, UUID_SIZE); +} + static int ext4_update_primary_sb(struct super_block *sb, handle_t *handle, ext4_update_sb_callback func, @@ -1131,6 +1141,67 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label return 0; } +static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi, + struct fsuuid __user *ufsuuid) +{ + int ret = 0; + struct fsuuid fsuuid; + __u8 uuid[UUID_SIZE]; + + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) + return -EFAULT; + + if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0) + return -EINVAL; + + lock_buffer(sbi->s_sbh); + memcpy(uuid, sbi->s_es->s_uuid, UUID_SIZE); + unlock_buffer(sbi->s_sbh); + + if (copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE)) + return -EFAULT; + return 0; +} + +static int ext4_ioctl_setuuid(struct file *filp, + const struct fsuuid __user *ufsuuid) +{ + int ret = 0; + struct super_block *sb = file_inode(filp)->i_sb; + struct fsuuid fsuuid; + __u8 uuid[UUID_SIZE]; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + /* + * If any checksums (group descriptors or metadata) are being used + * then the checksum seed feature is required to change the UUID. + */ + if (((ext4_has_feature_gdt_csum(sb) || ext4_has_metadata_csum(sb)) + && !ext4_has_feature_csum_seed(sb)) + || ext4_has_feature_stable_inodes(sb)) + return -EOPNOTSUPP; + + if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid))) + return -EFAULT; + + if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0) + return -EINVAL; + + if (copy_from_user(uuid, &ufsuuid->fsu_uuid[0], UUID_SIZE)) + return -EFAULT; + + ret = mnt_want_write_file(filp); + if (ret) + return ret; + + ret = ext4_update_superblocks_fn(sb, ext4_sb_setuuid, &uuid); + mnt_drop_write_file(filp); + + return ret; +} + static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { struct inode *inode = file_inode(filp); @@ -1509,6 +1580,10 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return ext4_ioctl_setlabel(filp, (const void __user *)arg); + case EXT4_IOC_GETFSUUID: + return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg); + case EXT4_IOC_SETFSUUID: + return ext4_ioctl_setuuid(filp, (const void __user *)arg); default: return -ENOTTY; } @@ -1586,6 +1661,8 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) case EXT4_IOC_CHECKPOINT: case FS_IOC_GETFSLABEL: case FS_IOC_SETFSLABEL: + case EXT4_IOC_GETFSUUID: + case EXT4_IOC_SETFSUUID: break; default: return -ENOIOCTLCMD;