Re: [patch V2 1/8] signal: Prevent exec() race

[Frederic Weisbecker <frederic@kernel.org>]

Le Mon, Sep 07, 2026 at 05:26:04PM +0200, Thomas Gleixner a écrit :
> On Mon, Sep 07 2026 at 14:31, Frederic Weisbecker wrote:
> > Le Sat, Sep 05, 2026 at 08:59:01PM +0200, Thomas Gleixner a écrit :
> >> -out:
> >> -	spin_unlock_irq(&tsk->sighand->siglock);
> >> +	flush_sigqueue_list(&sigq_list);
> >
> > It probably doesn't matter in practice, I don't know feel free to ignore,
> > but FWIW it looks like it's still vulnerable to the theoretical far fetched
> > race I described. The head is moved under the lock but individual nodes are
> > deleted without the lock.
> >
> > CPU 0                                CPU 1                   CPU 2
> > -----                                -----                   -----
> >
> > exit_signals()
> >    spin_lock(sighand)
> >    tsk->flags |= PF_EXITING;
> >    list_splice_init(&queue->list, head);
> >    spin_unlock(sighand)
> >
> >    list_for_each_safe(head, node)
> >       list_del_init(node)
> >          node->next = node // A
> >          node->prev = node // B
> > ...
> >                                      de_thread()
> >                                         // acquired tsk->flags
> >                                         // and signal flushed
> >                                         // through tasklist_lock
> >                                         transfer_pid() // C
> >                                                            
> >                                                            posix_timer_fn()
> >                                                               posixtimer_send_sigqueue()
> >                                                                  // OBSERVES C
> >                                                                  t = posixtimer_get_target(tmr)
> >                                                                  lock_task_sighand()
> >                                                                     // OBSERVES A
> >                                                                     if (!list_empty(q))
> >                                                                        // BUT NOT B
> >                                                                        list_add_tail(q) // D
> >
> > Then who knows which write wins, B or D?
> 
> For a moment you almost convinced me, but that's not possible:
> 
>     de_thread()
>         ....
> 
> 	if (!thread_leader()) {
>             wait_until(old_leader->exit_state);
> 
>             transfer_pid();
> 
> old_leader sets the exit_state in exit_notify():
> 
>     do_exit()
>       exit_signals()
>         lock(sighand)
>           old_leader->flags |= PF_EXITING;
>           head = remove_signals()
>         unlock(sighand)
>         flush_list(head)
>      ...
>      exit_notify()
>        old_leader->exit_state = EXIT_XXX;
> 
> From a program order POV the flush is completed _before_ the new leader
> can observe old_leader->exit_state and swap TIDS. exit_notify() and the
> wait in de_thread() are serialized via tasklist_lock.

Yes on that side all is program order. But the ordering is not mirrored on
the other side (at this stage of the patchset).

> 
> The signal is either dropped before transfer_pid() is observable due to
> PF_EXITING on the old leader or queued on the new leader and then
> discarded in posixtimer_exit() -> flush_itimer_signals().
> 
> The only valid question is whether it is guaranteed that on a weakly
> ordered system the stores in flush_sigqueue_list() are visible _before_
> transfer_pid() is visible to the third party.
> 
> It's not obvious of course and might deserve a comment.
> 
>       exit_signals()
>         lock(sighand)
>           old_leader->flags |= PF_EXITING;
>           head = remove_signals()
> #1      // RELEASE: PF_EXITING must become visible
>         unlock(sighand)
>         flush_list(head)
> 
>      ...
>      posixtimer_exit()
>        posix_cpu_timers_exit_task()
>         lock(sighand)
>         ...
> #2      // RELEASE: The stores in flush_list() must become visible
>         //          They might be already in case of preemption
>         //	    or due a RELEASE operation in seccomp_filter_release()
>         unlock(sighand)

That second step only appears at the end of the patchset, right? Otherwise
it's done on release_task(), which is after transfer_pid().

> 
>     ...
>     exit_notify()
>       lock(task_list_lock)
>       exit_state = EXIT_ZOMBIE;
> #3    // RELEASE: exit_state must become visible
>       unlock(task_list_lock)
> 
> So the new leader cannot proceed before #3 which means it can't swap
> TIDs before that point. That requires task_list_lock so there is no way
> that the TID swap can trickle before the lock is held and exit_state
> being non-zero.
> 
> Though the important part is that the third party on CPU3 has to acquire
> sighand lock in posixtimer_send_sigqueue(), which is an ACQUIRE
> operation. That means _all_ accesses to tsk::flags and to the sigqueue
> must happen _after_ the lock is acquired.
> 
> If it acquires it after #1 and before the TID swap it must observe
> PF_EXITING and return immediately. So a concurrent modification of
> timer::sigqueue in flush_list() or not-yet visible stores are
> irrelevant.
> 
> If it acquires it after #2 it must observe the full writes to the
> sigqueue. So after that point it does not longer matter whether the PID
> resolves to T1 or T2.
> 
> No?

At the end of the patchset yes. But it doesn't look that way in this
very patch which is to be backported alone.

Thanks.

-- 
Frederic Weisbecker
SUSE Labs