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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875xkm60m6.ffs@tglx>
Date: Thu, 06 Mar 2025 09:10:09 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Anna-Maria Behnsen
 <anna-maria@...utronix.de>, Benjamin Segall <bsegall@...gle.com>, Eric
 Dumazet <edumazet@...gle.com>, Andrey Vagin <avagin@...nvz.org>, Pavel
 Tikhomirov <ptikhomirov@...tuozzo.com>, Peter Zijlstra
 <peterz@...radead.org>
Subject: Re: [patch V2 01/17] posix-timers: Initialise timer before adding
 it to the hash table

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