2023-08-03 02:41:43

by zhanchengbin

[permalink] [raw]
Subject: [RFC PATCH v2 0/2] ext4: Fix the conflict between modifying the superblock in user mode and kernel mode

Tune2fs does not recognize writes to filesystems in another namespace. Two
simultaneous write operations on a block will lead to file system
inconsistency, because there is no lock protection between userland and
kernelland.

The operation is as follows:
first terminal second terminal
mkfs.ext4 /dev/sdb;
mount /dev/sdb /test-sdb;
dd if=/dev/zero of=/test-sdb/test1 bs=1M count=100;
unshare -m;
umount;
gdb tune2fs;
b io_channel_write_byte
r -e remount-ro /dev/sdb
c(Write a byte of old data into the cache)
exit;
(gdb finish)
tune2fs -l /dev/sdb;
tune2fs 1.46.4 (18-Aug-2021)
tune2fs: Superblock checksum does not match superblock while trying to
open /dev/sdb
Couldn't find valid filesystem superblock.

Link: https://lore.kernel.org/linux-ext4/[email protected]/

After discussing with Tytso, I decided to use ioctl to solve the above
problems. These patches are an example I wrote to complete the modification
of the s_errors variable in the super block.

Finally, if you have any good ideas, welcome to communicate with me by
email.

Diff V2 vs V1:
1) Modify symbols of comment.
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
2) Add static symbols to functions.
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

zhanchengbin (2):
ext4: ioctl adds a framework for modifying superblock parameters
ext4: ioctl add EXT4_IOC_SUPERBLOCK_KEY_S_ERRORS

fs/ext4/ext4.h | 12 +++
fs/ext4/ioctl.c | 149 ++++++++++++++++++++++++++++++++++++++
include/uapi/linux/ext4.h | 28 +++++++
3 files changed, 189 insertions(+)

--
2.31.1



2023-08-03 02:42:01

by zhanchengbin

[permalink] [raw]
Subject: [RFC PATCH v2 1/2] ext4: ioctl adds a framework for modifying superblock parameters

A framework for modifying attrs in the super block is added here, including
the check and set of attrs in the super block, and it will be very
convenient to add new parameter modifications later.

When in use, pass in ext4_sbattrs to the kernel mode, and
ext4_sbattrs->sba_attrs contains attrs that need to be modified one by
one. It is guaranteed that all variables in one call are modified
atomically.

Link: https://lore.kernel.org/linux-ext4/[email protected]/
Signed-off-by: zhanchengbin <[email protected]>
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
---
fs/ext4/ext4.h | 12 ++++
fs/ext4/ioctl.c | 127 ++++++++++++++++++++++++++++++++++++++
include/uapi/linux/ext4.h | 26 ++++++++
3 files changed, 165 insertions(+)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0a2d55faa095..461d8bbe1e70 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3823,6 +3823,18 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
return buffer_uptodate(bh);
}

+struct ext4_ksbattrs {
+ __u32 sba_count;
+ struct ext4_sbattr *sba_attrs[];
+};
+
+struct ext4_sbattr_operation {
+ int sb_attr_key;
+ int (*sb_attr_check)(struct ext4_sbattr *p_sbattr);
+ void (*sb_attr_set)(struct ext4_super_block *es, const void *arg);
+};
+
+
#endif /* __KERNEL__ */

#define EFSBADCRC EBADMSG /* Bad CRC detected */
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 331859511f80..76653d855073 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -30,6 +30,13 @@
typedef void ext4_update_sb_callback(struct ext4_super_block *es,
const void *arg);

+/*
+ * Check and modify functions for each superblock variable
+ */
+struct ext4_sbattr_operation ext4_sbattr_ops[] = {
+ {EXT4_IOC_SUPERBLOCK_KEY_MAX, NULL, NULL},
+};
+
/*
* Superblock modification callback function for changing file system
* label
@@ -51,6 +58,18 @@ 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_superblock_attr(struct ext4_super_block *es, const void *arg)
+{
+ struct ext4_ksbattrs *p_sbattrs = (struct ext4_ksbattrs *)arg;
+ struct ext4_sbattr *p_sbattr;
+ int count;
+
+ for (count = 0; count < p_sbattrs->sba_count; count++) {
+ p_sbattr = p_sbattrs->sba_attrs[count];
+ ext4_sbattr_ops[p_sbattr->sba_key].sb_attr_set(es, p_sbattr);
+ }
+}
+
static
int ext4_update_primary_sb(struct super_block *sb, handle_t *handle,
ext4_update_sb_callback func,
@@ -1220,6 +1239,112 @@ static int ext4_ioctl_setuuid(struct file *filp,
return ret;
}

+/*
+ * Check the key-value pairs passed in from the user mode and assign it
+ * to the super block
+ */
+static int ext4_ioctl_set_superblock_attr(struct file *filp,
+ const struct ext4_sbattrs __user *usbattrs)
+{
+ int ret = 0;
+ struct super_block *sb = file_inode(filp)->i_sb;
+ struct ext4_sbattrs sbattrs, *p_sbattrs = NULL;
+ struct ext4_ksbattrs *p_ksbattrs = NULL;
+ struct ext4_sbattr sbattr, *p_sbattr = NULL;
+ size_t size;
+ int count = 0;
+
+ if (!capable(CAP_SYS_ADMIN)) {
+ ret = -EPERM;
+ goto failed;
+ }
+
+ if (copy_from_user(&sbattrs, usbattrs, sizeof(sbattrs))) {
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ if (sbattrs.sba_count > EXT4_SBATTR_MAX_COUNT) {
+ ret = -EINVAL;
+ goto failed;
+ }
+ size = sizeof(sbattrs) + sbattrs.sba_count * sizeof(struct ext4_sbarrt *);
+
+ p_sbattrs = kmalloc(size, GFP_KERNEL);
+ if (p_sbattrs == NULL) {
+ ret = -ENOMEM;
+ goto failed;
+ }
+
+ if (copy_from_user(p_sbattrs, usbattrs, size)) {
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ p_ksbattrs = kzalloc(size, GFP_KERNEL);
+ if (p_ksbattrs == NULL) {
+ ret = -ENOMEM;
+ goto failed;
+ }
+ p_ksbattrs->sba_count = p_sbattrs->sba_count;
+
+ while (count < p_sbattrs->sba_count) {
+ if (copy_from_user(&sbattr, p_sbattrs->sba_attrs[count], sizeof(sbattr))) {
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ size = sizeof(sbattr) + sbattr.sba_len * sizeof(char);
+ if (size > PAGE_SIZE) {
+ ret = -EINVAL;
+ goto failed;
+ }
+
+ if (sbattr.sba_key >= EXT4_IOC_SUPERBLOCK_KEY_MAX) {
+ ret = -EINVAL;
+ goto failed;
+ }
+
+ p_sbattr = kmalloc(size, GFP_KERNEL);
+ if (p_sbattr == NULL) {
+ ret = -ENOMEM;
+ goto failed;
+ }
+
+ p_ksbattrs->sba_attrs[count] = p_sbattr;
+
+ if (copy_from_user(p_sbattr, p_sbattrs->sba_attrs[count], size)) {
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ /* Check the validity of key-value pairs */
+ ret = ext4_sbattr_ops[sbattr.sba_key].sb_attr_check(p_sbattr);
+ if (ret)
+ goto failed;
+
+ count++;
+ }
+
+ ret = mnt_want_write_file(filp);
+ if (ret)
+ goto failed;
+
+ ret = ext4_update_superblocks_fn(sb, ext4_sb_set_superblock_attr, p_ksbattrs);
+ mnt_drop_write_file(filp);
+
+failed:
+ kfree(p_sbattrs);
+
+ if (p_ksbattrs) {
+ for (count = 0; count < p_ksbattrs->sba_count; count++)
+ kfree(p_ksbattrs->sba_attrs[count]);
+ kfree(p_ksbattrs);
+ }
+
+ return ret;
+}
+
static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
struct inode *inode = file_inode(filp);
@@ -1607,6 +1732,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_SUPERBLOCK_ATTR:
+ return ext4_ioctl_set_superblock_attr(filp, (const void __user *)arg);
default:
return -ENOTTY;
}
diff --git a/include/uapi/linux/ext4.h b/include/uapi/linux/ext4.h
index 1c4c2dd29112..79b5a751b83e 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_SUPERBLOCK_ATTR _IOW('f', 45, struct ext4_sbattrs)

#define EXT4_IOC_SHUTDOWN _IOR('X', 125, __u32)

@@ -69,6 +70,31 @@
EXT4_IOC_CHECKPOINT_FLAG_ZEROOUT | \
EXT4_IOC_CHECKPOINT_FLAG_DRY_RUN)

+/*
+ * Structure for EXT4_IOC_SET_SUPERBLOCK_ATTR
+ */
+struct ext4_sbattrs {
+ __u32 sba_count; /* attrs number */
+ struct ext4_sbattr __user *sba_attrs[];
+};
+
+/*
+ * key for attr's magic in superblock
+ * len for attr's size in superblock
+ * value for attr's value in superblock
+ */
+struct ext4_sbattr {
+ __u32 sba_key;
+ __u32 sba_len;
+ char sba_value[];
+};
+
+enum ext4_ioc_superblock_key {
+ EXT4_IOC_SUPERBLOCK_KEY_MAX = 0,
+};
+
+#define EXT4_SBATTR_MAX_COUNT 20
+
/*
* Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
*/
--
2.31.1


2023-08-03 02:42:29

by zhanchengbin

[permalink] [raw]
Subject: [RFC PATCH v2 2/2] ext4: ioctl add EXT4_IOC_SUPERBLOCK_KEY_S_ERRORS

Added modifications to s_errors in the modification framework of
superblock's attrs. There is no lock protection when the user mode
operates on the same block, so I modify the s_errors by the modification
framework of superblock's attrs.

The parameters passed in from the user mode will be checked by
sb_attr_errors_check first, if the check is passed, It will call
buffer_lock in ext4_update_superblocks_fn and use sb_attr_errors_set, so
it can ensure the atomicity of a modification.

Signed-off-by: zhanchengbin <[email protected]>
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
---
fs/ext4/ioctl.c | 22 ++++++++++++++++++++++
include/uapi/linux/ext4.h | 4 +++-
2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 76653d855073..4053a038da0a 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -30,10 +30,32 @@
typedef void ext4_update_sb_callback(struct ext4_super_block *es,
const void *arg);

+static int sb_attr_errors_check(struct ext4_sbattr *p_sbattr)
+{
+ __u16 error_behavior = *(__u16 *)(p_sbattr->sba_value);
+
+ if (p_sbattr->sba_len != EXT4_IOC_SUPERBLOCK_LEN_S_ERRORS)
+ return -EINVAL;
+
+ if (error_behavior < EXT4_ERRORS_CONTINUE ||
+ error_behavior > EXT4_ERRORS_PANIC)
+ return -EINVAL;
+
+ return 0;
+}
+
+static void sb_attr_errors_set(struct ext4_super_block *es, const void *arg)
+{
+ struct ext4_sbattr *p_sbattr = (struct ext4_sbattr *)arg;
+
+ es->s_errors = cpu_to_le16(*(__u16 *)p_sbattr->sba_value);
+}
+
/*
* Check and modify functions for each superblock variable
*/
struct ext4_sbattr_operation ext4_sbattr_ops[] = {
+ {EXT4_IOC_SUPERBLOCK_KEY_S_ERRORS, sb_attr_errors_check, sb_attr_errors_set},
{EXT4_IOC_SUPERBLOCK_KEY_MAX, NULL, NULL},
};

diff --git a/include/uapi/linux/ext4.h b/include/uapi/linux/ext4.h
index 79b5a751b83e..f5429630343e 100644
--- a/include/uapi/linux/ext4.h
+++ b/include/uapi/linux/ext4.h
@@ -90,8 +90,10 @@ struct ext4_sbattr {
};

enum ext4_ioc_superblock_key {
- EXT4_IOC_SUPERBLOCK_KEY_MAX = 0,
+ EXT4_IOC_SUPERBLOCK_KEY_S_ERRORS = 0,
+ EXT4_IOC_SUPERBLOCK_KEY_MAX,
};
+#define EXT4_IOC_SUPERBLOCK_LEN_S_ERRORS 2

#define EXT4_SBATTR_MAX_COUNT 20

--
2.31.1