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

[Frederic Weisbecker <frederic@kernel.org>]

On Thu, Mar 06, 2025 at 09:10:09AM +0100, Thomas Gleixner wrote:
> 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?

Right that's why I added that in my proposed fix and it's the really important
part.

As for the rest, it's just a matter of taste. Checking
timer::it_signal && timer::list instead of flipping a bit in a pointer and
rely on magic comparison seemed to me clearer and easier to follow but that's
just a proposal, I won't mind either way.

Thanks.

> 
> Thanks,
> 
>         tglx