Re: [RFC PATCH 2/2] kernel/module: Decouple klp and ftrace from load_module

[Song Chen <chensong_2000@126.com>]

Hi,


On 4/20/26 10:27, Masami Hiramatsu (Google) wrote:
> On Thu, 16 Apr 2026 16:49:32 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
>> 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, chensong_2000@189.cn 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.
> 
> Agree.
> 
> If this needs to be ordered so strictly, why we will use a "single"
> module notifier chain for this complex situation?
> 
> I think the notifier call chain is just for notice a single signal,
> instead of sending several different signals, especially if there is
> any dependency among the callbacks.
> 
> If notification callbacks need to be ordered, they are currently
> sorted by representing priority numerically, but this is quite
> fragile for updating. It has to look up other registered priorities
> and adjust the order among dependencies each time. For this reason,
> this mechanism is not suitable for global ordering. (It's like line
> numbers in BASIC.)
> It is probably only useful for representing dependencies between
> two components maintained by the same maintainer.
> 
> I'm against a general-purpose system that makes everything modular.
> It unnecessarily complicates things. If there are processes that
> require strict ordering, especially processes that must be performed
> before each stage as part of the framework, they should be called
> directly from the framework, not via notification callbacks.
> 
> This makes it simpler and more robust to maintain.
> 
> Only the framework's end users should utilize notification callbacks.
> 
> Thank you,
> 
> 

my motivation is to decouple ftrace and klp from module loader and make 
blocking_notifier_chain more generic, but it doesn't become generic 
completely. I understand your and Petr's comments and agree.

Thanks

Best regards

Song

>>
>> Best Regards,
>> Petr
>>
> 
>