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