[PATCH 1/5] exec: kill unsafe BUG_ON(sig->count) checks

[PATCH 1/5] exec: kill unsafe BUG_ON(sig->count) checks

[Oleg Nesterov]

de_thread:

	if (atomic_read(&oldsighand->count) <= 1)
		BUG_ON(atomic_read(&sig->count) != 1);

This is not safe without the rmb() in between. The results of two correctly
ordered __exit_signal()->atomic_dec_and_test()'s could be seen out of order
on our CPU.

The same is true for the "thread_group_empty()" case, __unhash_process()'s
changes could be seen before atomic_dec_and_test(&sig->count).

On some platforms (including i386) atomic_read() doesn't provide even the
compiler barrier, in that case these checks are simply racy.

Remove these BUG_ON()'s. Alternatively, we can do something like

	BUG_ON( ({ smp_rmb(); atomic_read(&sig->count) != 1; }) );

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- t/fs/exec.c~1_BUG_ON	2007-08-18 17:36:58.000000000 +0400
+++ t/fs/exec.c	2007-08-18 18:19:41.000000000 +0400
@@ -784,7 +784,6 @@ static int de_thread(struct task_struct 
 	 * and we can just re-use it all.
 	 */
 	if (atomic_read(&oldsighand->count) <= 1) {
-		BUG_ON(atomic_read(&sig->count) != 1);
 		signalfd_detach(tsk);
 		exit_itimers(sig);
 		return 0;
@@ -929,8 +928,6 @@ no_thread_group:
 	if (leader)
 		release_task(leader);
 
-	BUG_ON(atomic_read(&sig->count) != 1);
-
 	if (atomic_read(&oldsighand->count) == 1) {
 		/*
 		 * Now that we nuked the rest of the thread group,

Re: [PATCH 1/5] exec: kill unsafe BUG_ON(sig->count) checks

[Paul E. McKenney]

On Sat, Aug 18, 2007 at 09:39:36PM +0400, Oleg Nesterov wrote:
> de_thread:
> 
> 	if (atomic_read(&oldsighand->count) <= 1)
> 		BUG_ON(atomic_read(&sig->count) != 1);
> 
> This is not safe without the rmb() in between. The results of two correctly
> ordered __exit_signal()->atomic_dec_and_test()'s could be seen out of order
> on our CPU.
> 
> The same is true for the "thread_group_empty()" case, __unhash_process()'s
> changes could be seen before atomic_dec_and_test(&sig->count).
> 
> On some platforms (including i386) atomic_read() doesn't provide even the
> compiler barrier, in that case these checks are simply racy.
> 
> Remove these BUG_ON()'s. Alternatively, we can do something like
> 
> 	BUG_ON( ({ smp_rmb(); atomic_read(&sig->count) != 1; }) );

Good catches!

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- t/fs/exec.c~1_BUG_ON	2007-08-18 17:36:58.000000000 +0400
> +++ t/fs/exec.c	2007-08-18 18:19:41.000000000 +0400
> @@ -784,7 +784,6 @@ static int de_thread(struct task_struct 
>  	 * and we can just re-use it all.
>  	 */
>  	if (atomic_read(&oldsighand->count) <= 1) {
> -		BUG_ON(atomic_read(&sig->count) != 1);
>  		signalfd_detach(tsk);
>  		exit_itimers(sig);
>  		return 0;
> @@ -929,8 +928,6 @@ no_thread_group:
>  	if (leader)
>  		release_task(leader);
> 
> -	BUG_ON(atomic_read(&sig->count) != 1);
> -
>  	if (atomic_read(&oldsighand->count) == 1) {
>  		/*
>  		 * Now that we nuked the rest of the thread group,
> 

Re: [PATCH 1/5] exec: kill unsafe BUG_ON(sig->count) checks

[Roland McGrath]

Those BUG_ON's were there because of past bugs and fragility in the
de_thread and exit synchronization stuff.  There's no real need to leave
them (in fixed form) if we are confident of that stuff working right now,
or have other assertions to give us that confidence when we change it.


Thanks,
Roland