On Sat, Jul 08, 2023 at 03:29:51PM +0800, zhanchengbin wrote:
> On 2023/7/5 3:33, Theodore Ts'o wrote:
>
> I have written the ioctl for EXT4_IOC_SET_ERROR_BEHAVIOR according to your
> instructions and verified that it can indeed modify the data on the disk.
>
> However, I realized some problems. If we use the ioctl method to modify the
> data, it would require multiple ioctls in user space to modify the
> superblock.
> If one ioctl successfully modifies the superblock data, but another ioctl
> fails, the atomicity of the superblock cannot be guaranteed. This is just
> within one user space process, not to mention the scenario of multiple user
> space processes calling ioctls concurrently. Additionally, multiple ioctls
> modifying the superblock may be placed in multiple transactions, which
> further
> compromises atomicity.
>
> Writing the entire superblock buffer_head to disk can ensure atomicity.
...at a cost of racing with the mounted fs, which might be updating the
superblock at the same time; and prohibiting the kernel devs from
closing the "scribble on mounted bdev" attack surface.
How many of these attributes do you /really/ need to be able to commit
atomically, anyway? If the system crashes, can't you re-run the
program and end up with the same super fields?
--D
> However, these data need to be converted to ext4_sb_info. Otherwise, during
> unmount, the data in memory will overwrite the data on the disk.
> (Modifying the values in ext4_sb_info can potentially introduce unexpected
> issues, just like the problem thata arises when setting the UUID without
> checking ext4_has_feature_csum_seed.)
>
> > So the better approach is to have multiple new ioctl's for each
> > superblock field (or set of fields) that you might want modify. We
> > have some of these already --- for example, EXT4_IOC_SETFSUUID and
> > FS_IOC_SETFSLABEL. For tune2fs, all of additional ioctls for making
> > configuration changes while the file system is mounted are:
> >
> > * EXT4_IOC_SET_FEATURES
> > - for tune2fs -O...
> > * EXT4_IOC_CLEAR_FEATURES
> > - for tune2fs -O^...
> > * EXT4_IOC_SET_ERROR_BEHAVIOR
> > - for tune2fs -e
> > * EXT4_IOC_SET_DEFAULT_MOUNT_FLAGS
> > - for tune2fs -o
> > * EXT4_IOC_SET_DEFAULT_MOUNT_OPTS
> > - for tune2fs -E mount_opts=XXX
> > * EXT4_IOC_SET_FSCK_POLICY
> > - for tune2fs -[cCiT]
> > * EXT4_IOC_SET_RESERVED_ALLOC
> > - for tune2fs -[ugm]
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 331859511f80..d598e1975786 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -51,6 +51,11 @@ static void ext4_sb_setuuid(struct ext4_super_block *es,
> const void *arg)
> memcpy(es->s_uuid, (__u8 *)arg, UUID_SIZE);
> }
>
> +static void ext4_sb_set_error_behavior(struct ext4_super_block *es, const
> void *arg)
> +{
> + es->s_errors = cpu_to_le16(*(__u16 *)arg);
> +}
> +
> static
> int ext4_update_primary_sb(struct super_block *sb, handle_t *handle,
> ext4_update_sb_callback func,
> @@ -1220,6 +1225,32 @@ static int ext4_ioctl_setuuid(struct file *filp,
> return ret;
> }
>
> +static int ext4_ioctl_set_error_behavior(struct file *filp,
> + const __u16 __user *uerror_behavior)
> +{
> + int ret = 0;
> + struct super_block *sb = file_inode(filp)->i_sb;
> + __u16 error_behavior;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (copy_from_user(&error_behavior, uerror_behavior,
> sizeof(error_behavior)))
> + return -EFAULT;
> +
> + if (error_behavior < EXT4_ERRORS_CONTINUE || error_behavior >
> EXT4_ERRORS_PANIC)
> + return -EINVAL;
> +
> + ret = mnt_want_write_file(filp);
> + if (ret)
> + return ret;
> +
> + ret = ext4_update_superblocks_fn(sb, ext4_sb_set_error_behavior,
> &error_behavior);
> + 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);
> @@ -1607,6 +1638,8 @@ static long __ext4_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
> return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg);
> case EXT4_IOC_SETFSUUID:
> return ext4_ioctl_setuuid(filp, (const void __user *)arg);
> + case EXT4_IOC_SET_ERROR_BEHAVIOR:
> + return ext4_ioctl_set_error_behavior(filp, (const void __user *)arg);
> default:
> return -ENOTTY;
> }
> diff --git a/include/uapi/linux/ext4.h b/include/uapi/linux/ext4.h
> index 1c4c2dd29112..68329d51a8a7 100644
> --- a/include/uapi/linux/ext4.h
> +++ b/include/uapi/linux/ext4.h
> @@ -33,6 +33,7 @@
> #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_SET_ERROR_BEHAVIOR _IOW('f', 45, __u16)
>
> #define EXT4_IOC_SHUTDOWN _IOR('X', 125, __u32)
>
>
>
> Thanks,
> - bin.
>
