linux-kernel - Re: [PATCH v2 2/2] livepatch: Replace tasklist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250225183308.yjtgdl3esisvlhab@jpoimboe>
Date: Tue, 25 Feb 2025 10:33:08 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Yafang Shao <laoar.shao@...il.com>
Cc: jikos@...nel.org, mbenes@...e.cz, pmladek@...e.com,
	joe.lawrence@...hat.com, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] livepatch: Replace tasklist_lock with RCU

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