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

[Eric W. Biederman]

Thomas Gleixner <tglx@xxxxxxxxxx> writes:

> On Fri, Aug 28 2026 at 00:56, Thomas Gleixner wrote:
>> On Thu, Aug 27 2026 at 13:43, Eric W. Biederman wrote:
>>> The obvious place to clean up task::pending i.e. signals is in
>>> exit_signals().
>>>
>>> I expect if I read through the history again that I would find that
>>> exit_signals() used to call flush_sigqueue, and that during the addition
>>> of posix thread signal handling flush_sigqueue was moved into
>>> __exit_signal in release_task because knowing if the entire thread group
>>> is dead was not available during that part of 2.5.
>>>
>>> We should honor PF_EXITING on a task and simply stop delivering
>>> signals to it.  Today the code goes halfway there and does not
>>> set sig-pending after PF_EXITING is set.
>>
>> If flushing tsk::pending in exit_signals() is safe and stopping signals
>> to be queued when PF_EXITING is observed under sighand lock, then sure
>> that's the right thing to do. I'll look into that tomorrow.
>
> By some definition of tomorrow. :)
>
> Something like the below?

Yes.

It all comes after
   ptrace_event(PTRACE_EVENT_EXIT, code) and
   coredump_task_exit(tsk, core_state)
so should be completely invisible to userspace.

I don't see any problems with your proposed patch.

I am pondering what it would take to move
"flush_sigqueue(&p->signal->shared_pending);"
into do_exit in the group_dead case.

Perhaps exit_signals could perform the decrement of signal->live
and return group_dead.  If so all of the work could be performed
in exit_signals().  As a follow-on change of cource.

But before I even propose something like that I have another change
in this area that I need to post.

Eric



> ---
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -299,12 +299,13 @@ void release_task(struct task_struct *p)
>  	free_pids(post.pids);
>  	release_thread(p);
>  	/*
> -	 * This task was already removed from the process/thread/pid lists
> -	 * and lock_task_sighand(p) can't succeed. Nobody else can touch
> -	 * ->pending or, if group dead, signal->shared_pending. We can call
> -	 * flush_sigqueue() lockless.
> +	 * This task was already removed from the process/thread/pid lists and
> +	 * lock_task_sighand(p) can't succeed. If it's the group leader then
> +	 * flush tsk->signal->shared_pending. tsk->pending has been flushed
> +	 * already in exit_signals(). Nothing else can touch
> +	 * signal->shared_pending anymore, so flush_sigqueue() can be invoked
> +	 * lockless.
>  	 */
> -	flush_sigqueue(&p->pending);
>  	if (thread_group_leader(p))
>  		flush_sigqueue(&p->signal->shared_pending);
>  
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1030,6 +1030,10 @@ static int __send_signal_locked(int sig,
>  	lockdep_assert_held(&t->sighand->siglock);
>  
>  	result = TRACE_SIGNAL_IGNORED;
> +
> +	if (unlikely(type == PIDTYPE_PID && (t->flags & PF_EXITING)))
> +		goto ret;
> +
>  	if (!prepare_signal(sig, t, force))
>  		goto ret;
>  
> @@ -1990,6 +1994,9 @@ void posixtimer_send_sigqueue(struct k_i
>  	if (!likely(lock_task_sighand(t, &flags)))
>  		return;
>  
> +	if (unlikely(tmr->it_pid_type == PIDTYPE_PID && (t->flags & PF_EXITING)))
> +		return;
> +
>  	/*
>  	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
>  	 * locked to prevent a race against dequeue_signal().
> @@ -3118,6 +3125,16 @@ static void retarget_shared_pending(stru
>  	}
>  }
>  
> +/*
> + * tsk::flags has PF_EXITING set which prevents signals to be queued for on
> + * tsk::pending. Nothing else can touch the tsk::pending anymore so it can be
> + * flushed lockless.
> + */
> +static inline void flush_pending_unlocked(struct task_struct *tsk)
> +{
> +	flush_sigqueue(&tsk->pending);
> +}
> +
>  void exit_signals(struct task_struct *tsk)
>  {
>  	int group_stop = 0;
> @@ -3130,8 +3147,10 @@ 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;
>  		cgroup_threadgroup_change_end(tsk);
> +		flush_pending_unlocked(tsk);
>  		return;
>  	}
>  
> @@ -3157,6 +3176,8 @@ void exit_signals(struct task_struct *ts
>  out:
>  	spin_unlock_irq(&tsk->sighand->siglock);
>  
> +	flush_pending_unlocked(tsk);
> +
>  	/*
>  	 * If group stop has completed, deliver the notification.  This
>  	 * should always go to the real parent of the group leader.