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

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> 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.

I agree that simply removing the special case that could skip grabbing
siglock is more maintainable.  Just one last thing to think about.

Oleg it appears you were the one who added the special case to skip
taking siglock.  So if you aren't worried about us removing it then
I am happy to see it go.

Eric


> 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.