2007-08-18 17:38:05

by Oleg Nesterov

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

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 <[email protected]>

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

2007-08-18 22:34:16

by Paul E. McKenney

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

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 <[email protected]>

> Signed-off-by: Oleg Nesterov <[email protected]>
>
> --- 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,
>

2007-08-18 23:29:20

by Roland McGrath

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

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