On Thu 2026-04-16 13:18:30, Petr Pavlu wrote:
> On 4/15/26 8:43 AM, Song Chen wrote:
> > On 4/14/26 22:33, Petr Pavlu wrote:
> >> On 4/13/26 10:07 AM, [email protected] wrote:
> >>> diff --git a/include/linux/module.h b/include/linux/module.h
> >>> index 14f391b186c6..0bdd56f9defd 100644
> >>> --- a/include/linux/module.h
> >>> +++ b/include/linux/module.h
> >>> @@ -308,6 +308,14 @@ enum module_state {
> >>> MODULE_STATE_COMING, /* Full formed, running module_init. */
> >>> MODULE_STATE_GOING, /* Going away. */
> >>> MODULE_STATE_UNFORMED, /* Still setting it up. */
> >>> + MODULE_STATE_FORMED,
> >>
> >> I don't see a reason to add a new module state. Why is it necessary and
> >> how does it fit with the existing states?
> >>
> > because once notifier fails in state MODULE_STATE_UNFORMED (now only ftrace
> > has someting to do in this state), notifier chain will roll back by calling
> > blocking_notifier_call_chain_robust, i'm afraid MODULE_STATE_GOING is going
> > to jeopardise the notifers which don't handle it appropriately, like:
> >
> > case MODULE_STATE_COMING:
> > kmalloc();
> > case MODULE_STATE_GOING:
> > kfree();
>
> My understanding is that the current module "state machine" operates as
> follows. Transitions marked with an asterisk (*) are announced via the
> module notifier.
>
> ---> UNFORMED --*> COMING --*> LIVE --*> GOING -.
> ^ | ^ |
> | '---------------------* |
> '---------------------------------------'
>
> The new code aims to replace the current ftrace_module_init() call in
> load_module(). To achieve this, it adds a notification for the UNFORMED
> state (only when loading a module) and introduces a new FORMED state for
> rollback. FORMED is purely a fake state because it never appears in
> module::state. The new structure is as follows:
>
> ,--*> (FORMED)
> |
> --*> UNFORMED --*> COMING --*> LIVE --*> GOING -.
> ^ | ^ |
> | '---------------------* |
> '---------------------------------------'
>
> I'm afraid this is quite complex and inconsistent. Unless it can be kept
> simple, we would be just replacing one special handling with a different
> complexity, which is not worth it.
> >>
> >>> + if (err)
> >>> + goto ddebug_cleanup;
> >>> /* Finally it's fully formed, ready to start executing. */
> >>> err = complete_formation(mod, info);
> >>> - if (err)
> >>> + if (err) {
> >>> + blocking_notifier_call_chain_reverse(&module_notify_list,
> >>> + MODULE_STATE_FORMED, mod);
> >>> goto ddebug_cleanup;
> >>> + }
> >>> - err = prepare_coming_module(mod);
> >>> + err = prepare_module_state_transaction(mod,
> >>> + MODULE_STATE_COMING, MODULE_STATE_GOING);
> >>> if (err)
> >>> goto bug_cleanup;
> >>> @@ -3522,7 +3519,6 @@ static int load_module(struct load_info *info,
> >>> const char __user *uargs,
> >>> destroy_params(mod->kp, mod->num_kp);
> >>> blocking_notifier_call_chain(&module_notify_list,
> >>> MODULE_STATE_GOING, mod);
> >>
> >> My understanding is that all notifier chains for MODULE_STATE_GOING
> >> should be reversed.
> > yes, all, from lowest priority notifier to highest.
> > I will resend patch 1 which was failed due to my proxy setting.
>
> What I meant here is that the call:
>
> blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod);
>
> should be replaced with:
>
> blocking_notifier_call_chain_reverse(&module_notify_list, MODULE_STATE_GOING,
> mod);
>
> >
> >>
> >>> - klp_module_going(mod);
> >>> bug_cleanup:
> >>> mod->state = MODULE_STATE_GOING;
> >>> /* module_bug_cleanup needs module_mutex protection */
> >>
> >> The patch removes the klp_module_going() cleanup call in load_module().
> >> Similarly, the ftrace_release_mod() call under the ddebug_cleanup label
> >> should be removed and appropriately replaced with a cleanup via
> >> a notifier.
> >>
> > err = prepare_module_state_transaction(mod,
> > MODULE_STATE_UNFORMED, MODULE_STATE_FORMED);
> > if (err)
> > goto ddebug_cleanup;
> >
> > ftrace will be cleanup in blocking_notifier_call_chain_robust rolling back.
> >
> > err = prepare_module_state_transaction(mod,
> > MODULE_STATE_COMING, MODULE_STATE_GOING);
> >
> > each notifier including ftrace and klp will be cleanup in
> > blocking_notifier_call_chain_robust rolling back.
> >
> > if all notifiers are successful in MODULE_STATE_COMING, they all will be
> > clean up in
> > coming_cleanup:
> > mod->state = MODULE_STATE_GOING;
> > destroy_params(mod->kp, mod->num_kp);
> > blocking_notifier_call_chain(&module_notify_list,
> > MODULE_STATE_GOING, mod);
> >
> > if something wrong underneath.
>
> My point is that the patch leaves a call to ftrace_release_mod() in
> load_module(), which I expected to be handled via a notifier.
I think that I have got it. The ftrace code needs two notifiers when
the module is being loaded and two when it is going.
This is why Sond added the new state. But I think that we would
need two new states to call:
+ ftrace_module_init() in MODULE_STATE_UNFORMED
+ ftrace_module_enable() in MODULE_STATE_FORMED
and
+ ftrace_free_mem() in MODULE_STATE_PRE_GOING
+ ftrace_free_mem() in MODULE_STATE_GOING
By using the ascii art:
-*> UNFORMED -*> FORMED -> COMING -*> LIVE -*> PRE_GOING -*> GOING -.
| | | ^ ^ ^
| | '----------------' | |
| '--------------------------------------' |
'------------------------------------------------------'
But I think that this is not worth it.
Best Regards,
Petr
