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.
--
Thanks,
Petr
