Re: [patch V2 01/17] posix-timers: Initialise timer before adding it to the hash table

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Mar 05 2025 at 18:25, Frederic Weisbecker wrote:
> Le Sun, Mar 02, 2025 at 08:36:44PM +0100, Thomas Gleixner a écrit :
> Looking at this more or less lockless whole thing again, is the
> ordering between creation and subsequent operations sufficiently guaranteed?
>
>     T0                                                T1
> ---------                                             -----------
> do_timer_create()
>     posix_timer_add()
>         spin_lock(hash_lock)
>         // A
>         timer->it_id = ...
>         spin_unlock(hash_lock)
>     // Initialize timer fields
>     // B
>     new_timer->.... = ....
>     common_timer_create()
>         // C
>         hrtimer_init()
>     spin_lock(current->sighand)
>     // D
>     WRITE_ONCE(new_timer->it_signal, current->signal)
>     spin_unlock(current->sighand)
>                                                       do_timer_settime()
>                                                           lock_timer()
>                                                               // observes A && D
>                                                               posix_timer_by_id()
>                                                               spin_lock_irqsave(&timr->it_lock)
>                                                               // recheck ok
>                                                               if (timr->it_signal == current->signal)
>                                                                   return timr
>                                                               common_timer_get()
>                                                                   // fiddle with timer fields
>                                                                   // but doesn't observe B
>                                                                   // for example doesn't observe SIGEV_NONE
>                                                                   sig_none = timr->it_sigev_notify == SIGEV_NONE;
>                                                                   ...
>                                                                   // doesn't observe C
>                                                                   // hrtimer_init() isn't visible yet
>                                                                   // It might mess up after the hrtimer_start()
>                                                                   hrtimer_start()

Pretty far fetched and I did not think it fully through whether it can
really happen. But that's trivial enough to solve without this
hlist_hashed() indirection:

+      spin_lock(new_timer->lock);
       spin_lock(current->sighand);
       WRITE_ONCE(new_timer->it_signal, current->signal);
       spin_unlock(current->sighand);
+      spin_unlock(new_timer->lock);

Simply because the release of timer::lock guarantees that the memory
operations before the release have been completed before the release
completes.

Consequently the other CPU must observe a consistent set A - D after it
acquired the lock.

No?

Thanks,

        tglx