Re: [PATCH V2] signal: Prevent exec() race

[Oleg Nesterov]

as for the change on exit_signal,

On 09/01, Thomas Gleixner wrote:
>
> @@ -3120,6 +3137,7 @@ static void retarget_shared_pending(stru
>
>  void exit_signals(struct task_struct *tsk)
>  {
> +	LIST_HEAD(sigq_list);
>  	int group_stop = 0;
>  	sigset_t unblocked;
>
> @@ -3130,8 +3148,12 @@ void exit_signals(struct task_struct *ts
>  	cgroup_threadgroup_change_begin(tsk);
>
>  	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
> -		tsk->flags |= PF_EXITING;
> +		scoped_guard(spinlock_irq, &tsk->sighand->siglock) {
> +			tsk->flags |= PF_EXITING;
> +			sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
> +		}
>  		cgroup_threadgroup_change_end(tsk);
> +		flush_sigqueue_list(&sigq_list);
>  		return;
>  	}
>
> @@ -3141,6 +3163,7 @@ void exit_signals(struct task_struct *ts
>  	 * see wants_signal(), do_signal_stop().
>  	 */
>  	tsk->flags |= PF_EXITING;
> +	sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
>
>  	cgroup_threadgroup_change_end(tsk);
>
> @@ -3157,6 +3180,8 @@ void exit_signals(struct task_struct *ts
>  out:
>  	spin_unlock_irq(&tsk->sighand->siglock);
>
> +	flush_sigqueue_list(&sigq_list);
> +
>  	/*
>  	 * If group stop has completed, deliver the notification.  This
>  	 * should always go to the real parent of the group leader.

This is subjective and mostly cosmetic, but what do you think
about the alternative change below?

I won't insist, but to me both the patch and resulting code look
a bit simpler this way.

Oleg.
---

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3120,6 +3120,7 @@ static void retarget_shared_pending(struct task_struct *tsk, sigset_t *which)
 
 void exit_signals(struct task_struct *tsk)
 {
+	LIST_HEAD(sigq_list);
 	int group_stop = 0;
 	sigset_t unblocked;
 
@@ -3129,21 +3130,18 @@ void exit_signals(struct task_struct *tsk)
 	 */
 	cgroup_threadgroup_change_begin(tsk);
 
-	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->flags |= PF_EXITING;
-		cgroup_threadgroup_change_end(tsk);
-		return;
-	}
-
 	spin_lock_irq(&tsk->sighand->siglock);
 	/*
 	 * From now this task is not visible for group-wide signals,
 	 * see wants_signal(), do_signal_stop().
 	 */
 	tsk->flags |= PF_EXITING;
+	sigqueue_dequeue_pending(&tsk->pending, &sigq_list);
 
 	cgroup_threadgroup_change_end(tsk);
 
+	if (thread_group_empty(tsk) || (tsk->signal->flags & SIGNAL_GROUP_EXIT))
+		goto out;
 	if (!task_sigpending(tsk))
 		goto out;
 
@@ -3157,6 +3155,7 @@ void exit_signals(struct task_struct *tsk)
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
+	flush_sigqueue_list(&sigq_list);
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.