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

[IBM]

Runyu Xiao <runyu.xiao@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Assisted-by: Codex:GPT-5
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
> ---
>  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