linux-kernel - Re: [PATCH] kthread: Fix the race condition when kthread is parked

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: <53AC9114.9070106@codeaurora.org>
Date:	Thu, 26 Jun 2014 14:31:00 -0700
From:	Subbaraman Narayanamurthy <subbaram@...eaurora.org>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kthread: Fix the race condition when kthread is parked

On 06/25/14 17:43, Thomas Gleixner wrote:
> The kthread park/unpark logic has the following issue:
>
> Task   CPU 0				CPU 1
>
> T1     unplug cpu1
>         kthread_park(T2)
>         set_bit(KTHREAD_SHOULD_PARK);
> 	  wait_for_completion()
> T2					parkme(X)
> 				   	  __set_current_state(TASK_PARKED);
> 				   	  while (test_bit(KTHREAD_SHOULD_PARK)) {
> 				     	    if (!test_and_set_bit(KTHREAD_IS_PARKED))
> 				              complete();
> 			             	    schedule();
> T1   plug cpu1
>
> --> premature wakeup of T2, i.e. before unpark, so T2 gets scheduled on
>      CPU 0
I understood the explanation above. But still I don't understand how 
this premature wakeup of T2 is happening/possible? Also, what will 
happen if the task state is not in TASK_PARKED when __kthread_unpark is 
called? __kthread_bind will fail silently causing the same problem.
> Reorder the logic so that the unplug code binds the thread to the
> target cpu before clearing the KTHREAD_SHOULD_PARK bit.
>
> Reported-by: Subbaraman Narayanamurthy<subbaram@...eaurora.org>
> Signed-off-by: Thomas Gleixner<tglx@...utronix.de>
> Cc:stable@...r.kernel.org
>
> ---
>   kernel/kthread.c |   14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> Index: linux/kernel/kthread.c
> ===================================================================
> --- linux.orig/kernel/kthread.c
> +++ linux/kernel/kthread.c
> @@ -382,6 +382,15 @@ struct task_struct *kthread_create_on_cp
>   
>   static void __kthread_unpark(struct task_struct *k, struct kthread *kthread)
>   {
> +	/*
> +	 * Rebind the thread to the target cpu first if it is a per
> +	 * cpu thread unconditionally because it must be bound to the
> +	 * target cpu before it can observe the KTHREAD_SHOULD_PARK
> +	 * bit cleared.
> +	 */
> +	if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> +		__kthread_bind(k, kthread->cpu, TASK_PARKED);
> +
>   	clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
>   	/*
>   	 * We clear the IS_PARKED bit here as we don't wait
> @@ -389,11 +398,8 @@ static void __kthread_unpark(struct task
>   	 * park before that happens we'd see the IS_PARKED bit
>   	 * which might be about to be cleared.
>   	 */
> -	if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> -		if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> -			__kthread_bind(k, kthread->cpu, TASK_PARKED);
> +	if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags))
>   		wake_up_state(k, TASK_PARKED);
> -	}
>   }
>   
>   /**
>
>
>
>
>
>
Thanks for the patch. I've tested (running hotplug tests) it for 
sometime and looks good so far. Can you please submit it?

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/