* [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
@ 2024-01-02 13:37 Baokun Li
2024-01-03 9:06 ` Jan Kara
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Baokun Li @ 2024-01-02 13:37 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
In the following concurrency we will access the uninitialized rs->lock:
ext4_fill_super
ext4_register_sysfs
// sysfs registered msg_ratelimit_interval_ms
// Other processes modify rs->interval to
// non-zero via msg_ratelimit_interval_ms
ext4_orphan_cleanup
ext4_msg(sb, KERN_INFO, "Errors on filesystem, "
__ext4_msg
___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state)
if (!rs->interval) // do nothing if interval is 0
return 1;
raw_spin_trylock_irqsave(&rs->lock, flags)
raw_spin_trylock(lock)
_raw_spin_trylock
__raw_spin_trylock
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_)
lock_acquire
__lock_acquire
register_lock_class
assign_lock_key
dump_stack();
ratelimit_state_init(&sbi->s_msg_ratelimit_state, 5 * HZ, 10);
raw_spin_lock_init(&rs->lock);
// init rs->lock here
and get the following dump_stack:
=========================================================
INFO: trying to register non-static key.
The code is fine but needs lockdep annotation, or maybe
you didn't initialize this object before use?
turning off the locking correctness validator.
CPU: 12 PID: 753 Comm: mount Tainted: G E 6.7.0-rc6-next-20231222 #504
[...]
Call Trace:
dump_stack_lvl+0xc5/0x170
dump_stack+0x18/0x30
register_lock_class+0x740/0x7c0
__lock_acquire+0x69/0x13a0
lock_acquire+0x120/0x450
_raw_spin_trylock+0x98/0xd0
___ratelimit+0xf6/0x220
__ext4_msg+0x7f/0x160 [ext4]
ext4_orphan_cleanup+0x665/0x740 [ext4]
__ext4_fill_super+0x21ea/0x2b10 [ext4]
ext4_fill_super+0x14d/0x360 [ext4]
[...]
=========================================================
Normally interval is 0 until s_msg_ratelimit_state is initialized, so
___ratelimit() does nothing. But registering sysfs precedes initializing
rs->lock, so it is possible to change rs->interval to a non-zero value
via the msg_ratelimit_interval_ms interface of sysfs while rs->lock is
uninitialized, and then a call to ext4_msg triggers the problem by
accessing an uninitialized rs->lock. Therefore register sysfs after all
initializations are complete to avoid such problems.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/super.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 0980845c8b8f..1db23b0e8a4f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5564,19 +5564,15 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (err)
goto failed_mount6;
- err = ext4_register_sysfs(sb);
- if (err)
- goto failed_mount7;
-
err = ext4_init_orphan_info(sb);
if (err)
- goto failed_mount8;
+ goto failed_mount7;
#ifdef CONFIG_QUOTA
/* Enable quota usage during mount. */
if (ext4_has_feature_quota(sb) && !sb_rdonly(sb)) {
err = ext4_enable_quotas(sb);
if (err)
- goto failed_mount9;
+ goto failed_mount8;
}
#endif /* CONFIG_QUOTA */
@@ -5602,7 +5598,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
ext4_msg(sb, KERN_INFO, "recovery complete");
err = ext4_mark_recovery_complete(sb, es);
if (err)
- goto failed_mount10;
+ goto failed_mount9;
}
if (test_opt(sb, DISCARD) && !bdev_max_discard_sectors(sb->s_bdev))
@@ -5619,15 +5615,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
atomic_set(&sbi->s_warning_count, 0);
atomic_set(&sbi->s_msg_count, 0);
+ /* Register sysfs after all initializations are complete. */
+ err = ext4_register_sysfs(sb);
+ if (err)
+ goto failed_mount9;
+
return 0;
-failed_mount10:
+failed_mount9:
ext4_quotas_off(sb, EXT4_MAXQUOTAS);
-failed_mount9: __maybe_unused
+failed_mount8: __maybe_unused
ext4_release_orphan_info(sb);
-failed_mount8:
- ext4_unregister_sysfs(sb);
- kobject_put(&sbi->s_kobj);
failed_mount7:
ext4_unregister_li_request(sb);
failed_mount6:
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
2024-01-02 13:37 [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super() Baokun Li
@ 2024-01-03 9:06 ` Jan Kara
2024-02-27 7:51 ` Baokun Li
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2024-01-03 9:06 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3
On Tue 02-01-24 21:37:30, Baokun Li wrote:
> In the following concurrency we will access the uninitialized rs->lock:
>
> ext4_fill_super
> ext4_register_sysfs
> // sysfs registered msg_ratelimit_interval_ms
> // Other processes modify rs->interval to
> // non-zero via msg_ratelimit_interval_ms
> ext4_orphan_cleanup
> ext4_msg(sb, KERN_INFO, "Errors on filesystem, "
> __ext4_msg
> ___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state)
> if (!rs->interval) // do nothing if interval is 0
> return 1;
> raw_spin_trylock_irqsave(&rs->lock, flags)
> raw_spin_trylock(lock)
> _raw_spin_trylock
> __raw_spin_trylock
> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_)
> lock_acquire
> __lock_acquire
> register_lock_class
> assign_lock_key
> dump_stack();
> ratelimit_state_init(&sbi->s_msg_ratelimit_state, 5 * HZ, 10);
> raw_spin_lock_init(&rs->lock);
> // init rs->lock here
>
> and get the following dump_stack:
>
> =========================================================
> INFO: trying to register non-static key.
> The code is fine but needs lockdep annotation, or maybe
> you didn't initialize this object before use?
> turning off the locking correctness validator.
> CPU: 12 PID: 753 Comm: mount Tainted: G E 6.7.0-rc6-next-20231222 #504
> [...]
> Call Trace:
> dump_stack_lvl+0xc5/0x170
> dump_stack+0x18/0x30
> register_lock_class+0x740/0x7c0
> __lock_acquire+0x69/0x13a0
> lock_acquire+0x120/0x450
> _raw_spin_trylock+0x98/0xd0
> ___ratelimit+0xf6/0x220
> __ext4_msg+0x7f/0x160 [ext4]
> ext4_orphan_cleanup+0x665/0x740 [ext4]
> __ext4_fill_super+0x21ea/0x2b10 [ext4]
> ext4_fill_super+0x14d/0x360 [ext4]
> [...]
> =========================================================
>
> Normally interval is 0 until s_msg_ratelimit_state is initialized, so
> ___ratelimit() does nothing. But registering sysfs precedes initializing
> rs->lock, so it is possible to change rs->interval to a non-zero value
> via the msg_ratelimit_interval_ms interface of sysfs while rs->lock is
> uninitialized, and then a call to ext4_msg triggers the problem by
> accessing an uninitialized rs->lock. Therefore register sysfs after all
> initializations are complete to avoid such problems.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/super.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 0980845c8b8f..1db23b0e8a4f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5564,19 +5564,15 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> if (err)
> goto failed_mount6;
>
> - err = ext4_register_sysfs(sb);
> - if (err)
> - goto failed_mount7;
> -
> err = ext4_init_orphan_info(sb);
> if (err)
> - goto failed_mount8;
> + goto failed_mount7;
> #ifdef CONFIG_QUOTA
> /* Enable quota usage during mount. */
> if (ext4_has_feature_quota(sb) && !sb_rdonly(sb)) {
> err = ext4_enable_quotas(sb);
> if (err)
> - goto failed_mount9;
> + goto failed_mount8;
> }
> #endif /* CONFIG_QUOTA */
>
> @@ -5602,7 +5598,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> ext4_msg(sb, KERN_INFO, "recovery complete");
> err = ext4_mark_recovery_complete(sb, es);
> if (err)
> - goto failed_mount10;
> + goto failed_mount9;
> }
>
> if (test_opt(sb, DISCARD) && !bdev_max_discard_sectors(sb->s_bdev))
> @@ -5619,15 +5615,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> atomic_set(&sbi->s_warning_count, 0);
> atomic_set(&sbi->s_msg_count, 0);
>
> + /* Register sysfs after all initializations are complete. */
> + err = ext4_register_sysfs(sb);
> + if (err)
> + goto failed_mount9;
> +
> return 0;
>
> -failed_mount10:
> +failed_mount9:
> ext4_quotas_off(sb, EXT4_MAXQUOTAS);
> -failed_mount9: __maybe_unused
> +failed_mount8: __maybe_unused
> ext4_release_orphan_info(sb);
> -failed_mount8:
> - ext4_unregister_sysfs(sb);
> - kobject_put(&sbi->s_kobj);
> failed_mount7:
> ext4_unregister_li_request(sb);
> failed_mount6:
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
2024-01-02 13:37 [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super() Baokun Li
2024-01-03 9:06 ` Jan Kara
@ 2024-02-27 7:51 ` Baokun Li
2024-04-23 1:41 ` Baokun Li
2024-05-09 14:42 ` Theodore Ts'o
3 siblings, 0 replies; 6+ messages in thread
From: Baokun Li @ 2024-02-27 7:51 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, Baokun Li
A gentle ping.
On 2024/1/2 21:37, Baokun Li wrote:
> In the following concurrency we will access the uninitialized rs->lock:
>
> ext4_fill_super
> ext4_register_sysfs
> // sysfs registered msg_ratelimit_interval_ms
> // Other processes modify rs->interval to
> // non-zero via msg_ratelimit_interval_ms
> ext4_orphan_cleanup
> ext4_msg(sb, KERN_INFO, "Errors on filesystem, "
> __ext4_msg
> ___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state)
> if (!rs->interval) // do nothing if interval is 0
> return 1;
> raw_spin_trylock_irqsave(&rs->lock, flags)
> raw_spin_trylock(lock)
> _raw_spin_trylock
> __raw_spin_trylock
> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_)
> lock_acquire
> __lock_acquire
> register_lock_class
> assign_lock_key
> dump_stack();
> ratelimit_state_init(&sbi->s_msg_ratelimit_state, 5 * HZ, 10);
> raw_spin_lock_init(&rs->lock);
> // init rs->lock here
>
> and get the following dump_stack:
>
> =========================================================
> INFO: trying to register non-static key.
> The code is fine but needs lockdep annotation, or maybe
> you didn't initialize this object before use?
> turning off the locking correctness validator.
> CPU: 12 PID: 753 Comm: mount Tainted: G E 6.7.0-rc6-next-20231222 #504
> [...]
> Call Trace:
> dump_stack_lvl+0xc5/0x170
> dump_stack+0x18/0x30
> register_lock_class+0x740/0x7c0
> __lock_acquire+0x69/0x13a0
> lock_acquire+0x120/0x450
> _raw_spin_trylock+0x98/0xd0
> ___ratelimit+0xf6/0x220
> __ext4_msg+0x7f/0x160 [ext4]
> ext4_orphan_cleanup+0x665/0x740 [ext4]
> __ext4_fill_super+0x21ea/0x2b10 [ext4]
> ext4_fill_super+0x14d/0x360 [ext4]
> [...]
> =========================================================
>
> Normally interval is 0 until s_msg_ratelimit_state is initialized, so
> ___ratelimit() does nothing. But registering sysfs precedes initializing
> rs->lock, so it is possible to change rs->interval to a non-zero value
> via the msg_ratelimit_interval_ms interface of sysfs while rs->lock is
> uninitialized, and then a call to ext4_msg triggers the problem by
> accessing an uninitialized rs->lock. Therefore register sysfs after all
> initializations are complete to avoid such problems.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/ext4/super.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 0980845c8b8f..1db23b0e8a4f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5564,19 +5564,15 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> if (err)
> goto failed_mount6;
>
> - err = ext4_register_sysfs(sb);
> - if (err)
> - goto failed_mount7;
> -
> err = ext4_init_orphan_info(sb);
> if (err)
> - goto failed_mount8;
> + goto failed_mount7;
> #ifdef CONFIG_QUOTA
> /* Enable quota usage during mount. */
> if (ext4_has_feature_quota(sb) && !sb_rdonly(sb)) {
> err = ext4_enable_quotas(sb);
> if (err)
> - goto failed_mount9;
> + goto failed_mount8;
> }
> #endif /* CONFIG_QUOTA */
>
> @@ -5602,7 +5598,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> ext4_msg(sb, KERN_INFO, "recovery complete");
> err = ext4_mark_recovery_complete(sb, es);
> if (err)
> - goto failed_mount10;
> + goto failed_mount9;
> }
>
> if (test_opt(sb, DISCARD) && !bdev_max_discard_sectors(sb->s_bdev))
> @@ -5619,15 +5615,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> atomic_set(&sbi->s_warning_count, 0);
> atomic_set(&sbi->s_msg_count, 0);
>
> + /* Register sysfs after all initializations are complete. */
> + err = ext4_register_sysfs(sb);
> + if (err)
> + goto failed_mount9;
> +
> return 0;
>
> -failed_mount10:
> +failed_mount9:
> ext4_quotas_off(sb, EXT4_MAXQUOTAS);
> -failed_mount9: __maybe_unused
> +failed_mount8: __maybe_unused
> ext4_release_orphan_info(sb);
> -failed_mount8:
> - ext4_unregister_sysfs(sb);
> - kobject_put(&sbi->s_kobj);
> failed_mount7:
> ext4_unregister_li_request(sb);
> failed_mount6:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
2024-01-02 13:37 [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super() Baokun Li
2024-01-03 9:06 ` Jan Kara
2024-02-27 7:51 ` Baokun Li
@ 2024-04-23 1:41 ` Baokun Li
2024-05-03 2:09 ` Baokun Li
2024-05-09 14:42 ` Theodore Ts'o
3 siblings, 1 reply; 6+ messages in thread
From: Baokun Li @ 2024-04-23 1:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun
Gently ping again.
On 2024/1/2 21:37, Baokun Li wrote:
> In the following concurrency we will access the uninitialized rs->lock:
>
> ext4_fill_super
> ext4_register_sysfs
> // sysfs registered msg_ratelimit_interval_ms
> // Other processes modify rs->interval to
> // non-zero via msg_ratelimit_interval_ms
> ext4_orphan_cleanup
> ext4_msg(sb, KERN_INFO, "Errors on filesystem, "
> __ext4_msg
> ___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state)
> if (!rs->interval) // do nothing if interval is 0
> return 1;
> raw_spin_trylock_irqsave(&rs->lock, flags)
> raw_spin_trylock(lock)
> _raw_spin_trylock
> __raw_spin_trylock
> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_)
> lock_acquire
> __lock_acquire
> register_lock_class
> assign_lock_key
> dump_stack();
> ratelimit_state_init(&sbi->s_msg_ratelimit_state, 5 * HZ, 10);
> raw_spin_lock_init(&rs->lock);
> // init rs->lock here
>
> and get the following dump_stack:
>
> =========================================================
> INFO: trying to register non-static key.
> The code is fine but needs lockdep annotation, or maybe
> you didn't initialize this object before use?
> turning off the locking correctness validator.
> CPU: 12 PID: 753 Comm: mount Tainted: G E 6.7.0-rc6-next-20231222 #504
> [...]
> Call Trace:
> dump_stack_lvl+0xc5/0x170
> dump_stack+0x18/0x30
> register_lock_class+0x740/0x7c0
> __lock_acquire+0x69/0x13a0
> lock_acquire+0x120/0x450
> _raw_spin_trylock+0x98/0xd0
> ___ratelimit+0xf6/0x220
> __ext4_msg+0x7f/0x160 [ext4]
> ext4_orphan_cleanup+0x665/0x740 [ext4]
> __ext4_fill_super+0x21ea/0x2b10 [ext4]
> ext4_fill_super+0x14d/0x360 [ext4]
> [...]
> =========================================================
>
> Normally interval is 0 until s_msg_ratelimit_state is initialized, so
> ___ratelimit() does nothing. But registering sysfs precedes initializing
> rs->lock, so it is possible to change rs->interval to a non-zero value
> via the msg_ratelimit_interval_ms interface of sysfs while rs->lock is
> uninitialized, and then a call to ext4_msg triggers the problem by
> accessing an uninitialized rs->lock. Therefore register sysfs after all
> initializations are complete to avoid such problems.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/ext4/super.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 0980845c8b8f..1db23b0e8a4f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5564,19 +5564,15 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> if (err)
> goto failed_mount6;
>
> - err = ext4_register_sysfs(sb);
> - if (err)
> - goto failed_mount7;
> -
> err = ext4_init_orphan_info(sb);
> if (err)
> - goto failed_mount8;
> + goto failed_mount7;
> #ifdef CONFIG_QUOTA
> /* Enable quota usage during mount. */
> if (ext4_has_feature_quota(sb) && !sb_rdonly(sb)) {
> err = ext4_enable_quotas(sb);
> if (err)
> - goto failed_mount9;
> + goto failed_mount8;
> }
> #endif /* CONFIG_QUOTA */
>
> @@ -5602,7 +5598,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> ext4_msg(sb, KERN_INFO, "recovery complete");
> err = ext4_mark_recovery_complete(sb, es);
> if (err)
> - goto failed_mount10;
> + goto failed_mount9;
> }
>
> if (test_opt(sb, DISCARD) && !bdev_max_discard_sectors(sb->s_bdev))
> @@ -5619,15 +5615,17 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> atomic_set(&sbi->s_warning_count, 0);
> atomic_set(&sbi->s_msg_count, 0);
>
> + /* Register sysfs after all initializations are complete. */
> + err = ext4_register_sysfs(sb);
> + if (err)
> + goto failed_mount9;
> +
> return 0;
>
> -failed_mount10:
> +failed_mount9:
> ext4_quotas_off(sb, EXT4_MAXQUOTAS);
> -failed_mount9: __maybe_unused
> +failed_mount8: __maybe_unused
> ext4_release_orphan_info(sb);
> -failed_mount8:
> - ext4_unregister_sysfs(sb);
> - kobject_put(&sbi->s_kobj);
> failed_mount7:
> ext4_unregister_li_request(sb);
> failed_mount6:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
2024-04-23 1:41 ` Baokun Li
@ 2024-05-03 2:09 ` Baokun Li
0 siblings, 0 replies; 6+ messages in thread
From: Baokun Li @ 2024-05-03 2:09 UTC (permalink / raw)
To: linux-ext4, tytso
Cc: adilger.kernel, jack, ritesh.list, linux-kernel, yangerkun, Baokun Li
Hi Ted,
Would you consider merging in this patch in the current merge
window? This patch has been merged in our internal branch for
a while now and so far it is safe.
Regards,
Baokun
On 2024/4/23 9:41, Baokun Li wrote:
> Gently ping again.
>
> On 2024/1/2 21:37, Baokun Li wrote:
>> In the following concurrency we will access the uninitialized rs->lock:
>>
>> ext4_fill_super
>> ext4_register_sysfs
>> // sysfs registered msg_ratelimit_interval_ms
>> // Other processes modify rs->interval to
>> // non-zero via msg_ratelimit_interval_ms
>> ext4_orphan_cleanup
>> ext4_msg(sb, KERN_INFO, "Errors on filesystem, "
>> __ext4_msg
>> ___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state)
>> if (!rs->interval) // do nothing if interval is 0
>> return 1;
>> raw_spin_trylock_irqsave(&rs->lock, flags)
>> raw_spin_trylock(lock)
>> _raw_spin_trylock
>> __raw_spin_trylock
>> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_)
>> lock_acquire
>> __lock_acquire
>> register_lock_class
>> assign_lock_key
>> dump_stack();
>> ratelimit_state_init(&sbi->s_msg_ratelimit_state, 5 * HZ, 10);
>> raw_spin_lock_init(&rs->lock);
>> // init rs->lock here
>>
>> and get the following dump_stack:
>>
>> =========================================================
>> INFO: trying to register non-static key.
>> The code is fine but needs lockdep annotation, or maybe
>> you didn't initialize this object before use?
>> turning off the locking correctness validator.
>> CPU: 12 PID: 753 Comm: mount Tainted: G E 6.7.0-rc6-next-20231222 #504
>> [...]
>> Call Trace:
>> dump_stack_lvl+0xc5/0x170
>> dump_stack+0x18/0x30
>> register_lock_class+0x740/0x7c0
>> __lock_acquire+0x69/0x13a0
>> lock_acquire+0x120/0x450
>> _raw_spin_trylock+0x98/0xd0
>> ___ratelimit+0xf6/0x220
>> __ext4_msg+0x7f/0x160 [ext4]
>> ext4_orphan_cleanup+0x665/0x740 [ext4]
>> __ext4_fill_super+0x21ea/0x2b10 [ext4]
>> ext4_fill_super+0x14d/0x360 [ext4]
>> [...]
>> =========================================================
>>
>> Normally interval is 0 until s_msg_ratelimit_state is initialized, so
>> ___ratelimit() does nothing. But registering sysfs precedes initializing
>> rs->lock, so it is possible to change rs->interval to a non-zero value
>> via the msg_ratelimit_interval_ms interface of sysfs while rs->lock is
>> uninitialized, and then a call to ext4_msg triggers the problem by
>> accessing an uninitialized rs->lock. Therefore register sysfs after all
>> initializations are complete to avoid such problems.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>> fs/ext4/super.c | 22 ++++++++++------------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 0980845c8b8f..1db23b0e8a4f 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -5564,19 +5564,15 @@ static int __ext4_fill_super(struct
>> fs_context *fc, struct super_block *sb)
>> if (err)
>> goto failed_mount6;
>> - err = ext4_register_sysfs(sb);
>> - if (err)
>> - goto failed_mount7;
>> -
>> err = ext4_init_orphan_info(sb);
>> if (err)
>> - goto failed_mount8;
>> + goto failed_mount7;
>> #ifdef CONFIG_QUOTA
>> /* Enable quota usage during mount. */
>> if (ext4_has_feature_quota(sb) && !sb_rdonly(sb)) {
>> err = ext4_enable_quotas(sb);
>> if (err)
>> - goto failed_mount9;
>> + goto failed_mount8;
>> }
>> #endif /* CONFIG_QUOTA */
>> @@ -5602,7 +5598,7 @@ static int __ext4_fill_super(struct
>> fs_context *fc, struct super_block *sb)
>> ext4_msg(sb, KERN_INFO, "recovery complete");
>> err = ext4_mark_recovery_complete(sb, es);
>> if (err)
>> - goto failed_mount10;
>> + goto failed_mount9;
>> }
>> if (test_opt(sb, DISCARD) &&
>> !bdev_max_discard_sectors(sb->s_bdev))
>> @@ -5619,15 +5615,17 @@ static int __ext4_fill_super(struct
>> fs_context *fc, struct super_block *sb)
>> atomic_set(&sbi->s_warning_count, 0);
>> atomic_set(&sbi->s_msg_count, 0);
>> + /* Register sysfs after all initializations are complete. */
>> + err = ext4_register_sysfs(sb);
>> + if (err)
>> + goto failed_mount9;
>> +
>> return 0;
>> -failed_mount10:
>> +failed_mount9:
>> ext4_quotas_off(sb, EXT4_MAXQUOTAS);
>> -failed_mount9: __maybe_unused
>> +failed_mount8: __maybe_unused
>> ext4_release_orphan_info(sb);
>> -failed_mount8:
>> - ext4_unregister_sysfs(sb);
>> - kobject_put(&sbi->s_kobj);
>> failed_mount7:
>> ext4_unregister_li_request(sb);
>> failed_mount6:
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
2024-01-02 13:37 [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super() Baokun Li
` (2 preceding siblings ...)
2024-04-23 1:41 ` Baokun Li
@ 2024-05-09 14:42 ` Theodore Ts'o
3 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2024-05-09 14:42 UTC (permalink / raw)
To: linux-ext4, Baokun Li
Cc: Theodore Ts'o, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3
On Tue, 02 Jan 2024 21:37:30 +0800, Baokun Li wrote:
> In the following concurrency we will access the uninitialized rs->lock:
>
> ext4_fill_super
> ext4_register_sysfs
> // sysfs registered msg_ratelimit_interval_ms
> // Other processes modify rs->interval to
> // non-zero via msg_ratelimit_interval_ms
> ext4_orphan_cleanup
> ext4_msg(sb, KERN_INFO, "Errors on filesystem, "
> __ext4_msg
> ___ratelimit(&(EXT4_SB(sb)->s_msg_ratelimit_state)
> if (!rs->interval) // do nothing if interval is 0
> return 1;
> raw_spin_trylock_irqsave(&rs->lock, flags)
> raw_spin_trylock(lock)
> _raw_spin_trylock
> __raw_spin_trylock
> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_)
> lock_acquire
> __lock_acquire
> register_lock_class
> assign_lock_key
> dump_stack();
> ratelimit_state_init(&sbi->s_msg_ratelimit_state, 5 * HZ, 10);
> raw_spin_lock_init(&rs->lock);
> // init rs->lock here
>
> [...]
Applied, thanks!
[1/1] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super()
commit: b4b4fda34e535756f9e774fb2d09c4537b7dfd1c
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-09 14:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-02 13:37 [PATCH] ext4: fix uninitialized ratelimit_state->lock access in __ext4_fill_super() Baokun Li
2024-01-03 9:06 ` Jan Kara
2024-02-27 7:51 ` Baokun Li
2024-04-23 1:41 ` Baokun Li
2024-05-03 2:09 ` Baokun Li
2024-05-09 14:42 ` Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).