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]
Date:	Thu, 29 Aug 2013 17:23:27 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Libin <huawei.libin@...wei.com>
Cc:	akpm@...ux-foundation.org, tj@...nel.org, viro@...iv.linux.org.uk,
	eparis@...hat.com, tglx@...utronix.de, rusty@...tcorp.com.au,
	ebiederm@...ssion.com, john.stultz@...aro.org, mingo@...hat.com,
	peterz@...radead.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, lizefan@...wei.com,
	jovi.zhangwei@...wei.com, guohanjun@...wei.com,
	zhangdianfang@...wei.com, wangyijing@...wei.com
Subject: Re: [PATCH 09/14] rcutree: Fix invalid wakeup in rcu_wait

On Thu, Aug 29, 2013 at 09:57:44PM +0800, Libin wrote:
> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread is re-scheduled, calling set_current_state to set itself
> as state TASK_INTERRUPTIBLE, if it is preempted again after that and before
> __set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
> 
> To solve this problem, using preempt_disable() to bound the operaion that
> setting the task state and the conditions(set by the wake thread) validation.
> 
> Signed-off-by: Libin <huawei.libin@...wei.com>
> ---
>  kernel/rcutree.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/rcutree.h b/kernel/rcutree.h
> index b383258..e3f1278 100644
> --- a/kernel/rcutree.h
> +++ b/kernel/rcutree.h
> @@ -357,13 +357,17 @@ struct rcu_data {
> 
>  #define rcu_wait(cond)							\
>  do {									\
> +	preempt_disable();					\
>  	for (;;) {							\
>  		set_current_state(TASK_INTERRUPTIBLE);			\
>  		if (cond)						\
>  			break;						\

Here also the condition check follows the call to set_current_state(),
so this patch should not be needed.

Again, if you are seeing lost wakeups on this exact code path, please
let me know.

Please re-check your other patches.  If they really do follow the
buggy pattern you called out in your patch 0, namely the test preceding
the call to set_current_state(), please send patches that restore the
correct ordering.

Your use of preemption is not fixing anything.  It is at best hiding
the problem by making it less likely to occur.

							Thanx, Paul

> +		preempt_enable();				\
>  		schedule();						\
> +		preempt_disable();				\
>  	}								\
>  	__set_current_state(TASK_RUNNING);				\
> +	preempt_enable();				\
>  } while (0)
> 
>  /*
> -- 
> 1.8.2.1
> 
> 

--
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/

Powered by blists - more mailing lists