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

[Thomas Gleixner]

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.

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)

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

Thanks,

        tglx