Re: [PATCH] fix send_sigqueue() vs thread exit race

[Oleg Nesterov]

Thomas Gleixner wrote:
>
> On Mon, 2005-08-22 at 20:45 +0400, Oleg Nesterov wrote:
> >
> > kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after
> > that nobody can access this timer, so we don't need to lock timer->it_lock
> > at all in this case. No lock - no deadlock.
>
> It still deadlocks:
>
> CPU 0                           CPU 1
> write_lock(&tasklist_lock);
> __exit_signal()
>                                 timer expires
>                                 base->running_timer = timer
>                                   send_group_sigqueue()
>                                    read_lock(&tasklist_lock();
> exit_itimers()
>   del_timer_sync(timer)
>      waits for ever because           waits for ever on tasklist_lock
>      base->running_timer == timer

Silly me.

> I still think the last patch I sent is still necessary.

Thomas, you know that I like this change in __exit_{signal,sighand},
but i think this change is dangerous, should go in a separate patch,
and needs a lot of testing. But the decision is up to Ingo and Roland.

I am looking at your previous patch:

> -       read_lock(&tasklist_lock);
> +retry:
> +       if (unlikely(p->flags & PF_EXITING))
> +               return -1;
> +
> +       if (unlikely(!read_trylock(&tasklist_lock))) {
> +               cpu_relax();
> +               goto retry;
> +       }
> +       if (unlikely(p->flags & PF_EXITING)) {
> +               ret = -1;
> +               goto out_err;

What do you think about this:

int try_to_lock_this_beep_tasklist_lock(struct task_struct *group_leader)
{
	while (unlikely(!read_trylock(&tasklist_lock))) {
		if (group_leader->flags & PF_EXITING) {
			smp_rmb();
			if (thread_group_empty(group_leader))
				return 0;
		}
		cpu_relax();
	}

	return 1;
}

No need to re-check after we got tasklist, the signal will be flushed.
I think it's better to move the locking into the posix_timer_event, btw.
In that case we can drop my patch.

What is your opinion, can it work?

P.S.
     Thomas, thanks for explanation about posix-cpu-timers.

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