Re: [PATCH v2 2/2] livepatch: Replace tasklist_lock with RCU

[Josh Poimboeuf]

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