On Sun, Feb 23, 2025 at 02:20:46PM +0800, Yafang Shao wrote:
> +++ b/kernel/livepatch/patch.c
> @@ -95,7 +95,12 @@ static void notrace klp_ftrace_handler(unsigned long ip,
>
> patch_state = current->patch_state;
>
> - WARN_ON_ONCE(patch_state == KLP_TRANSITION_IDLE);
> + /* If the patch_state is KLP_TRANSITION_IDLE, it indicates the
> + * task was forked after klp_init_transition(). For this newly
> + * forked task, it is safe to switch it to klp_target_state.
> + */
> + if (patch_state == KLP_TRANSITION_IDLE)
> + current->patch_state = klp_target_state;
Hm, but then the following line is:
> if (patch_state == KLP_TRANSITION_UNPATCHED) {
Shouldn't the local 'patch_state' variable be updated?
It also seems unnecessary to update 'current->patch_state' here.
> @@ -294,6 +294,13 @@ static int klp_check_and_switch_task(struct task_struct
> *task, void *arg)
> {
> int ret;
>
> + /* If the patch_state remains KLP_TRANSITION_IDLE at this point, it
> + * indicates that the task was forked after klp_init_transition(). For
> + * this newly forked task, it is now safe to perform the switch.
> + */
> + if (task->patch_state == KLP_TRANSITION_IDLE)
> + goto out;
> +
This also seems unnecessary. No need to transition the patch if the
ftrace handler is already doing the right thing. klp_try_switch_task()
can just return early on !TIF_PATCH_PENDING.
> @@ -466,11 +474,11 @@ void klp_try_complete_transition(void)
> * Usually this will transition most (or all) of the tasks on a system
> * unless the patch includes changes to a very common function.
> */
> - read_lock(&tasklist_lock);
> + rcu_read_lock();
> for_each_process_thread(g, task)
> if (!klp_try_switch_task(task))
> complete = false;
> - read_unlock(&tasklist_lock);
> + rcu_read_unlock();
Can this also be done for the idle tasks?
--
Josh
