Re: [PATCH] Additional/catchup RCU signal fixes for -mm

[Oleg Nesterov <oleg@tv-sign.ru>]

"Paul E. McKenney" wrote:
>
> @@ -1386,7 +1387,7 @@ send_sigqueue(int sig, struct sigqueue *
>  {
>  	unsigned long flags;
>  	int ret = 0;
> -	struct sighand_struct *sh = p->sighand;
> +	struct sighand_struct *sh;
>
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>
> @@ -1405,7 +1406,15 @@ send_sigqueue(int sig, struct sigqueue *
>  		goto out_err;
>  	}
>
> +retry:
> +	sh = rcu_dereference(p->sighand);
> +
>  	spin_lock_irqsave(&sh->siglock, flags);
> +	if (p->sighand != sh) {
> +		/* We raced with exec() in a multithreaded process... */
> +		spin_unlock_irqrestore(&sh->siglock, flags);
> +		goto retry;

p->sighand can't be changed, de_thread calls exit_itimers() before
changing ->sighand. But I still think it can be NULL, and send_sigqueue()
should return -1 in that case.

> @@ -1464,15 +1473,8 @@ send_group_sigqueue(int sig, struct sigq
>
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>
> -	while (!read_trylock(&tasklist_lock)) {
> -		if (!p->sighand)
> -			return -1;
> -		cpu_relax();
> -	}
> -	if (unlikely(!p->sighand)) {
> -		ret = -1;
> -		goto out_err;
> -	}
> +	read_lock(&tasklist_lock);
> +	/* Since it_lock is held, p->sighand cannot be NULL. */
>  	spin_lock_irqsave(&p->sighand->siglock, flags);

Again, I think the comment is wrong.

However, now I think we really have a race with exec, and this race was not
introduced by your patches!

If !thread_group_leader() does exec de_thread() calls release_task(->group_leader)
before calling exit_itimers(). This means that send_group_sigqueue() which
always has p == ->group_leader parameter can oops here.

Oleg.