[PATCH] ext4: shut down error report timer on failed mount

[PATCH] ext4: shut down error report timer on failed mount

[Runyu Xiao]

__ext4_fill_super() arms s_err_report when the on-disk error count is
nonzero. If a later mount step fails, the failed-mount cleanup path uses
timer_delete_sync() before freeing sbi.

print_daily_error_info() rearms the timer when s_err_report_sec is nonzero,
so timer_delete_sync() does not prevent the timer from being queued again.
The rearmed callback can then access sbi after the failed mount has freed
it.

Use timer_shutdown_sync() for failed-mount cleanup. This matches the normal
unmount path and prevents the timer from being rearmed before sbi is freed.

Fixes: 66e61a9e9504 ("ext4: Once a day, printk file system error information to dmesg")
Cc: stable@vger.kernel.org
Assisted-by: Codex:GPT-5
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index bca0dc87d..154e00901 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5809,7 +5809,7 @@ failed_mount8: __maybe_unused
 	/* flush s_sb_upd_work before sbi destroy */
 	flush_work(&sbi->s_sb_upd_work);
 	ext4_stop_mmpd(sbi);
-	timer_delete_sync(&sbi->s_err_report);
+	timer_shutdown_sync(&sbi->s_err_report);
 	ext4_group_desc_free(sbi);
 failed_mount:
 #if IS_ENABLED(CONFIG_UNICODE)
-- 
2.34.1

Re: [PATCH] ext4: shut down error report timer on failed mount

[Ritesh Harjani]

Runyu Xiao <runyu.xiao@seu.edu.cn> writes:

> __ext4_fill_super() arms s_err_report when the on-disk error count is
> nonzero. If a later mount step fails, the failed-mount cleanup path uses
> timer_delete_sync() before freeing sbi.
>
> print_daily_error_info() rearms the timer when s_err_report_sec is nonzero,
> so timer_delete_sync() does not prevent the timer from being queued again.
> The rearmed callback can then access sbi after the failed mount has freed
> it.
>
> Use timer_shutdown_sync() for failed-mount cleanup. This matches the normal
> unmount path and prevents the timer from being rearmed before sbi is freed.
>

Did you hit any issue here? Sure timer_shutdown_sync() is safer variant
against a mod_timer() call by someone later, but I don't see what is the
issue here (maybe I am missing something).
I don't see why timer_delete_sync() on a failed unmount is not safe? The
mount itself has failed so there are not a lot of things which can cause
anything to trigger mod_timer() later? Do you have any example call
stack where we can hit this issue?


OTOH, I think we might have a problem in function err_report_sec_store()
though. It seems disabling the timer has a bug, since we are only
calling timer_delete_sync() there but we never make
sbi->s_err_report_sec = 0.
The comment says, 0 should disable the timer but seems it is not really
disabling it. Reading s_err_report_sec back is returning non-zero and if
we try to set the same value again - the timer is not re-started either.

	/* timeout in seconds for s_err_report; 0 disables the timer. */
	unsigned long s_err_report_sec;

Also, looking into ext4_update_super(), I think even the first error
doesn't re-arm the timer because of the wrong check.

		/*
		 * Start the daily error reporting function if it hasn't been
		 * started already and sbi->s_err_report_sec is not zero
		 */
		if (!es->s_error_count && !sbi->s_err_report_sec)
			mod_timer(&sbi->s_err_report,
					  jiffies + secs_to_jiffies(sbi->s_err_report_sec));
		le32_add_cpu(&es->s_error_count, sbi->s_add_error_count);

I guess that problem could be due to commit [1]
[1]:  d518215c2719 ("ext4: add sysfs attribute err_report_sec to control s_err_report timer")

Seems like multiple issues in there. Care to look at that path too?

-ritesh

> Fixes: 66e61a9e9504 ("ext4: Once a day, printk file system error information to dmesg")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT-5
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  fs/ext4/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index bca0dc87d..154e00901 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5809,7 +5809,7 @@ failed_mount8: __maybe_unused
>  	/* flush s_sb_upd_work before sbi destroy */
>  	flush_work(&sbi->s_sb_upd_work);
>  	ext4_stop_mmpd(sbi);
> -	timer_delete_sync(&sbi->s_err_report);
> +	timer_shutdown_sync(&sbi->s_err_report);
>  	ext4_group_desc_free(sbi);
>  failed_mount:
>  #if IS_ENABLED(CONFIG_UNICODE)
> -- 
> 2.34.1

Re: [PATCH] ext4: shut down error report timer on failed mount

[Runyu Xiao]

Hi Ritesh,

Thanks for reviewing this.

On your review, you wrote:
> Did you hit any issue here? Sure timer_shutdown_sync() is safer variant
> against a mod_timer() call by someone later, but I don't see what is the
> issue here (maybe I am missing something).
>
> I don't see why timer_delete_sync() on a failed unmount is not safe?
> ... Do you have any example call stack where we can hit this issue?
>
> OTOH, I think we might have a problem in function err_report_sec_store()
> though.
> ...
> I guess that problem could be due to commit d518215c2719 ...
> Seems like multiple issues in there. Care to look at that path too?

I rechecked the failed-mount path and could not find an independent rearm
path after timer_delete_sync() returns. The timer callback may rearm the
timer while it is running, but timer_delete_sync() waits for the callback to
finish and removes the timer rearmed by that callback before returning. The
failed-mount path also flushes s_sb_upd_work before deleting the timer, and
the ext4 sysfs attributes are registered only after the mount initialization
has completed successfully.

I did not hit a runtime failure here, and I cannot provide a reachable call
stack demonstrating the proposed race. I agree that the original patch does
not have sufficient evidence of a failed-mount timer race. Please consider
the v1 patch withdrawn; I will not send a v2.

I will investigate the err_report_sec_store() state update and the
ext4_update_super() condition separately, using d518215c2719 as the Fixes
commit if the history confirms that attribution.

Thanks for pointing this out.

Regards,
Runyu