Re: [PATCH] signal: Use list_del_init_careful() in flush_sigqueue()

[Oleg Nesterov <oleg@redhat.com>]

On 08/22, Hyunwoo Kim wrote:
>
> commit fb3bbcfe344e ("exit: change the release_task() paths to call
> flush_sigqueue() lockless") moved the ->pending flush from __exit_signal()
> to release_task(), where it runs without ->siglock. The justification was:
>
>   after the exiting task passes __exit_signal() lock_task_sighand() can't
>   succeed and pid_task(tmr->it_pid) will return NULL
>
> That second half does not hold for the old group leader in a non-leader
> exec(). de_thread() calls exchange_tids() before release_task(leader), so

Indeed... Thanks a lot!

I need some time to (try to ;) fully understand the problem and your fix...
I'll read your patch again tomorrow with a clear head.

Now... I hope that the next paragraph

    This means that after __exit_signal(tsk) nobody can play with tsk->pending
    or (if group_dead) with tsk->signal->shared_pending,

from the changelog is still true, so the only problem is that it is not
safe to play with q->list, right?

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -482,7 +482,11 @@ void flush_sigqueue(struct sigpending *queue)
>  	sigemptyset(&queue->signal);
>  	while (!list_empty(&queue->list)) {
>  		q = list_entry(queue->list.next, struct sigqueue , list);
> -		list_del_init(&q->list);
> +		/*
> +		 * Pairs with the list_empty() in posixtimer_send_sigqueue().
> +		 * release_task() gets here without ->siglock.
> +		 */
> +		list_del_init_careful(&q->list);
>  		__sigqueue_free(q);

Can't we avoid list_del_init() altogether? Can't flush_sigqueue() simply do

	list_for_each_entry(q, &pending->list, list)
		__sigqueue_free(q);

?

Oleg.