On Thu, Feb 13, 2025 at 9:36 AM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> On Wed, Feb 12, 2025 at 04:42:39PM +0100, Petr Mladek wrote:
> > CPU1 CPU1
> >
> > klp_try_complete_transition()
> >
> >
> > taskA:
> > + fork()
> > + klp_copy_process()
> > child->patch_state = KLP_PATCH_UNPATCHED
> >
> > klp_try_switch_task(taskA)
> > // safe
> >
> > child->patch_state = KLP_PATCH_PATCHED
> >
> > all processes patched
> >
> > klp_finish_transition()
> >
> >
> > list_add_tail_rcu(&p->thread_node,
> > &p->signal->thread_head);
> >
> >
> > BANG: The forked task has KLP_PATCH_UNPATCHED so that
> > klp_ftrace_handler() will redirect it to the old code.
> >
> > But CPU1 thinks that all tasks are migrated and is going
> > to finish the transition
>
>
> Maybe klp_try_complete_transition() could iterate the tasks in two
> passes? The first pass would use rcu_read_lock(). Then if all tasks
> appear to be patched, try again with tasklist_lock.
This option is much simpler, easier to understand, and more
maintainable. I prefer this approach.
>
> Or, we could do something completely different. There's no need for
> klp_copy_process() to copy the parent's state: a newly forked task can
> be patched immediately because it has no stack.
>
> So instead, just initialize it to KLP_TRANSITION_IDLE with
> TIF_PATCH_PENDING cleared. Then when klp_ftrace_handler() encounters a
> KLP_TRANSITION_IDLE task, it considers its state to be 'klp_target_state'.
>
> // called from copy_process()
> void klp_init_task(struct task_struct *child)
> {
> /* klp_ftrace_handler() will transition the task immediately */
> child->patch_state = KLP_TRANSITION_IDLE;
> clear_tsk_thread_flag(child, TIF_PATCH_PENDING);
> }
>
>
> klp_ftrace_handler():
>
> patch_state = current->patch_state;
>
> if (patch_state == KLP_TRANSITION_IDLE)
> patch_state = klp_target_state;
> ...
>
> Hm?
--
Regards
Yafang
