* [PATCH v2] ext4: add reserved GDT blocks check
@ 2022-06-01 9:27 Zhang Yi
2022-06-01 9:41 ` Jan Kara
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Zhang Yi @ 2022-06-01 9:27 UTC (permalink / raw)
To: linux-ext4; +Cc: tytso, adilger.kernel, jack, ritesh.list, yi.zhang, yukuai3
We capture a NULL pointer issue when resizing a corrupt ext4 image which
is freshly clear resize_inode feature (not run e2fsck). It could be
simply reproduced by following steps. The problem is because of the
resize_inode feature was cleared, and it will convert the filesystem to
meta_bg mode in ext4_resize_fs(), but the es->s_reserved_gdt_blocks was
not reduced to zero, so could we mistakenly call reserve_backup_gdb()
and passing an uninitialized resize_inode to it when adding new group
descriptors.
mkfs.ext4 /dev/sda 3G
tune2fs -O ^resize_inode /dev/sda #forget to run requested e2fsck
mount /dev/sda /mnt
resize2fs /dev/sda 8G
========
BUG: kernel NULL pointer dereference, address: 0000000000000028
CPU: 19 PID: 3243 Comm: resize2fs Not tainted 5.18.0-rc7-00001-gfde086c5ebfd #748
...
RIP: 0010:ext4_flex_group_add+0xe08/0x2570
...
Call Trace:
<TASK>
ext4_resize_fs+0xbec/0x1660
__ext4_ioctl+0x1749/0x24e0
ext4_ioctl+0x12/0x20
__x64_sys_ioctl+0xa6/0x110
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f2dd739617b
========
The fix is simple, add a check in ext4_resize_begin() to make sure that
the es->s_reserved_gdt_blocks is zero when the resize_inode feature is
disabled.
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
v2->v1:
- move check from ext4_resize_fs() to ext4_resize_begin().
fs/ext4/resize.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 90a941d20dff..8b70a4701293 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -53,6 +53,16 @@ int ext4_resize_begin(struct super_block *sb)
if (!capable(CAP_SYS_RESOURCE))
return -EPERM;
+ /*
+ * If the reserved GDT blocks is non-zero, the resize_inode feature
+ * should always be set.
+ */
+ if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks &&
+ !ext4_has_feature_resize_inode(sb)) {
+ ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero");
+ return -EFSCORRUPTED;
+ }
+
/*
* If we are not using the primary superblock/GDT copy don't resize,
* because the user tools have no way of handling this. Probably a
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext4: add reserved GDT blocks check
2022-06-01 9:27 [PATCH v2] ext4: add reserved GDT blocks check Zhang Yi
@ 2022-06-01 9:41 ` Jan Kara
2022-06-02 2:27 ` Ritesh Harjani
2022-06-18 2:59 ` Theodore Ts'o
2 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2022-06-01 9:41 UTC (permalink / raw)
To: Zhang Yi; +Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list, yukuai3
On Wed 01-06-22 17:27:17, Zhang Yi wrote:
> We capture a NULL pointer issue when resizing a corrupt ext4 image which
> is freshly clear resize_inode feature (not run e2fsck). It could be
> simply reproduced by following steps. The problem is because of the
> resize_inode feature was cleared, and it will convert the filesystem to
> meta_bg mode in ext4_resize_fs(), but the es->s_reserved_gdt_blocks was
> not reduced to zero, so could we mistakenly call reserve_backup_gdb()
> and passing an uninitialized resize_inode to it when adding new group
> descriptors.
>
> mkfs.ext4 /dev/sda 3G
> tune2fs -O ^resize_inode /dev/sda #forget to run requested e2fsck
> mount /dev/sda /mnt
> resize2fs /dev/sda 8G
>
> ========
> BUG: kernel NULL pointer dereference, address: 0000000000000028
> CPU: 19 PID: 3243 Comm: resize2fs Not tainted 5.18.0-rc7-00001-gfde086c5ebfd #748
> ...
> RIP: 0010:ext4_flex_group_add+0xe08/0x2570
> ...
> Call Trace:
> <TASK>
> ext4_resize_fs+0xbec/0x1660
> __ext4_ioctl+0x1749/0x24e0
> ext4_ioctl+0x12/0x20
> __x64_sys_ioctl+0xa6/0x110
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7f2dd739617b
> ========
>
> The fix is simple, add a check in ext4_resize_begin() to make sure that
> the es->s_reserved_gdt_blocks is zero when the resize_inode feature is
> disabled.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Looks good to me. Thanks for the fix. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> v2->v1:
> - move check from ext4_resize_fs() to ext4_resize_begin().
>
> fs/ext4/resize.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 90a941d20dff..8b70a4701293 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -53,6 +53,16 @@ int ext4_resize_begin(struct super_block *sb)
> if (!capable(CAP_SYS_RESOURCE))
> return -EPERM;
>
> + /*
> + * If the reserved GDT blocks is non-zero, the resize_inode feature
> + * should always be set.
> + */
> + if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks &&
> + !ext4_has_feature_resize_inode(sb)) {
> + ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero");
> + return -EFSCORRUPTED;
> + }
> +
> /*
> * If we are not using the primary superblock/GDT copy don't resize,
> * because the user tools have no way of handling this. Probably a
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext4: add reserved GDT blocks check
2022-06-01 9:27 [PATCH v2] ext4: add reserved GDT blocks check Zhang Yi
2022-06-01 9:41 ` Jan Kara
@ 2022-06-02 2:27 ` Ritesh Harjani
2022-06-18 2:59 ` Theodore Ts'o
2 siblings, 0 replies; 4+ messages in thread
From: Ritesh Harjani @ 2022-06-02 2:27 UTC (permalink / raw)
To: Zhang Yi; +Cc: linux-ext4, tytso, adilger.kernel, jack, yukuai3
On 22/06/01 05:27PM, Zhang Yi wrote:
> We capture a NULL pointer issue when resizing a corrupt ext4 image which
> is freshly clear resize_inode feature (not run e2fsck). It could be
> simply reproduced by following steps. The problem is because of the
> resize_inode feature was cleared, and it will convert the filesystem to
> meta_bg mode in ext4_resize_fs(), but the es->s_reserved_gdt_blocks was
> not reduced to zero, so could we mistakenly call reserve_backup_gdb()
> and passing an uninitialized resize_inode to it when adding new group
> descriptors.
>
> mkfs.ext4 /dev/sda 3G
> tune2fs -O ^resize_inode /dev/sda #forget to run requested e2fsck
> mount /dev/sda /mnt
> resize2fs /dev/sda 8G
>
> ========
> BUG: kernel NULL pointer dereference, address: 0000000000000028
> CPU: 19 PID: 3243 Comm: resize2fs Not tainted 5.18.0-rc7-00001-gfde086c5ebfd #748
> ...
> RIP: 0010:ext4_flex_group_add+0xe08/0x2570
> ...
> Call Trace:
> <TASK>
> ext4_resize_fs+0xbec/0x1660
> __ext4_ioctl+0x1749/0x24e0
> ext4_ioctl+0x12/0x20
> __x64_sys_ioctl+0xa6/0x110
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7f2dd739617b
> ========
>
> The fix is simple, add a check in ext4_resize_begin() to make sure that
> the es->s_reserved_gdt_blocks is zero when the resize_inode feature is
> disabled.
Sure, I have verified this change at my end too with your execerciser.
And having this check this in ext4_resize_begin(), looks good to me.
Feel free to add -
Reviewed-by: Ritesh Harjani <ritesh.list@gmail.com>
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> v2->v1:
> - move check from ext4_resize_fs() to ext4_resize_begin().
>
> fs/ext4/resize.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 90a941d20dff..8b70a4701293 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -53,6 +53,16 @@ int ext4_resize_begin(struct super_block *sb)
> if (!capable(CAP_SYS_RESOURCE))
> return -EPERM;
>
> + /*
> + * If the reserved GDT blocks is non-zero, the resize_inode feature
> + * should always be set.
> + */
> + if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks &&
> + !ext4_has_feature_resize_inode(sb)) {
> + ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero");
> + return -EFSCORRUPTED;
> + }
> +
> /*
> * If we are not using the primary superblock/GDT copy don't resize,
> * because the user tools have no way of handling this. Probably a
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext4: add reserved GDT blocks check
2022-06-01 9:27 [PATCH v2] ext4: add reserved GDT blocks check Zhang Yi
2022-06-01 9:41 ` Jan Kara
2022-06-02 2:27 ` Ritesh Harjani
@ 2022-06-18 2:59 ` Theodore Ts'o
2 siblings, 0 replies; 4+ messages in thread
From: Theodore Ts'o @ 2022-06-18 2:59 UTC (permalink / raw)
To: linux-ext4, Zhang Yi
Cc: Theodore Ts'o, ritesh.list, yukuai3, jack, adilger.kernel
On Wed, 1 Jun 2022 17:27:17 +0800, Zhang Yi wrote:
> We capture a NULL pointer issue when resizing a corrupt ext4 image which
> is freshly clear resize_inode feature (not run e2fsck). It could be
> simply reproduced by following steps. The problem is because of the
> resize_inode feature was cleared, and it will convert the filesystem to
> meta_bg mode in ext4_resize_fs(), but the es->s_reserved_gdt_blocks was
> not reduced to zero, so could we mistakenly call reserve_backup_gdb()
> and passing an uninitialized resize_inode to it when adding new group
> descriptors.
>
> [...]
Applied, thanks!
[1/1] ext4: add reserved GDT blocks check
commit: 7dc0ff3a33ea92cefaf032a6d0de9314a9a5fb20
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-18 2:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-01 9:27 [PATCH v2] ext4: add reserved GDT blocks check Zhang Yi
2022-06-01 9:41 ` Jan Kara
2022-06-02 2:27 ` Ritesh Harjani
2022-06-18 2:59 ` Theodore Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.