On Wed, 28 May 2025 21:22:37 +0800
yebin <[email protected]> wrote:
> This problem can be solved by releasing the 'mod_map' resource when the
> module is unloaded. Freeing up these resources is just an address that
> cannot be translated into symbols, and there are no worse consequences.
OK, I'm fine with releasing the mod_map resource without doing the text
modifications.
>
> > Again, the only solution to a ftrace_disable being set is a full reboot.
> >
> We can't ask users to know such specialized details of the
> implementation, which are unclear even to developers unfamiliar with the
> ftrace module. Users can accept planned reboot system recovery, but
> should not accept casual operations and the system crashes.All we can do
> is do a good job of protection, give users more tolerance.Perhaps a
> system that is dead but won't lie down is also a very undesirable
> situation.However, ftrace is used to collect information and locate
> faults. Even if it does not work, it does not affect services.In the
> production environment, the most afraid of using ftrace suddenly crashes
> the system.Therefore, the robustness of the tool itself is very important.
Preventing ftrace from crashing the system is the reason ftrace_disabled is
set and stops it from doing any more damage.
If you are worried about users not knowing that a reboot is necessary, we
could always add the "Dazed and confused, but trying to continue" message
that could also recommend a reboot.
> > -- Steve
> >
>
> I reworked the two patches, and the changes to the existing process
> should be minimal. I don't know if I can get your approval. If you
> agree, I'll post another V3 version.
>
> PATCH[1/2]:
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 51580e54677f..b3436d86e470 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7438,9 +7438,10 @@ void ftrace_release_mod(struct module *mod)
>
> mutex_lock(&ftrace_lock);
>
> - if (ftrace_disabled)
> - goto out_unlock;
> -
> + /*
> + * To avoid the UAF problem after the module is unloaded, the
> + * 'mod_map' resource needs to be released unconditionally.
> + */
> list_for_each_entry_safe(mod_map, n, &ftrace_mod_maps, list) {
> if (mod_map->mod == mod) {
> list_del_rcu(&mod_map->list);
> @@ -7451,6 +7452,9 @@ void ftrace_release_mod(struct module *mod)
> }
> }
>
> + if (ftrace_disabled)
> + goto out_unlock;
> +
> /*
> * Each module has its own ftrace_pages, remove
> * them from the list.
Yes, this is more appropriate.
Thanks,
-- Steve
>
> PATCH[2/2]:
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index a3d4dfad0cbc..51580e54677f 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7629,6 +7629,9 @@ allocate_ftrace_mod_map(struct module *mod,
> {
> struct ftrace_mod_map *mod_map;
>
> + if (ftrace_disabled)
> + return NULL;
> +
> mod_map = kmalloc(sizeof(*mod_map), GFP_KERNEL);
> if (!mod_map)
> return NULL;
