Re: [RFC][PATCH] Avoid race w/ posix-cpu-timer and exiting tasks

Oleg Nesterov
2006-06-13T22:49:11Z

john stultz wrote:
>
> Hey Ingo,
> We've occasionally come across OOPSes in posix-cpu-timer thread (as
> well as tripping over the BUG_ON(tsk->exit_state there) where it appears
> the task we're processing exits out on us while we're using it.
>
> Thus this fix tries to avoid running the posix-cpu-timers on a task that
> is exiting.
>
> --- 2.6-rt/kernel/posix-cpu-timers.c 2006-06-11 15:38:58.000000000 -0500
> +++ devrt/kernel/posix-cpu-timers.c 2006-06-12 10:52:20.000000000 -0500
> @@ -1290,12 +1290,15 @@
>
> #undef UNEXPIRED
>
> - BUG_ON(tsk->exit_state);
> -
> /*
> * Double-check with locks held.
> */
> read_lock(&tasklist_lock);
> + /* Make sure the task doesn't exit under us. */
> + if(unlikely(tsk->exit_state)) {
> + read_unlock(&tasklist_lock);
> + return;
> + }
> spin_lock(&tsk->sighand->siglock);

I strongly believe this BUG_ON() is indeed wrong, and I did a similar patch
a long ago:

[PATCH] posix-timers: remove false BUG_ON() from run_posix_cpu_timers()
Commit 3de463c7d9d58f8cf3395268230cb20a4c15bffa

However it was reverted due to some unclear problems (I think those problems
were not related to this patch).

Instead this patch was added:

[PATCH] Yet more posix-cpu-timer fixes
Commit 3de463c7d9d58f8cf3395268230cb20a4c15bffa

and I still think this patch is not correct.

Quoting myself:
>
> Roland McGrath wrote:
> >
> > @@ -566,6 +566,9 @@ static void arm_timer(struct k_itimer *t
> > struct cpu_timer_list *next;
> > unsigned long i;
> >
> > + if (CPUCLOCK_PERTHREAD(timer->it_clock) && (p->flags & PF_EXITING))
> > + return;
> > +
>
> Why CPUCLOCK_PERTHREAD() ?.
>
> Also, this is racy, no? Why should arm_timer() see PF_EXITING which is
> set on another cpu without any barriers/locking? After all, arm_timer()
> can test PF_EXITING before do_exit() sets this flag, but set ->it_xxx_expires
> after do_exit() resets it.

Oleg.