linux-kernel - Re: [patch V2 10/17] posix-timers: Make signal_struct::next_posix_timer_id an atomic

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: <87ikoo53xy.ffs@tglx>
Date: Wed, 05 Mar 2025 08:31:21 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Cyrill Gorcunov <gorcunov@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Anna-Maria Behnsen
 <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>,
 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 10/17] posix-timers: Make
 signal_struct::next_posix_timer_id an atomic_t

On Wed, Mar 05 2025 at 01:16, Cyrill Gorcunov wrote:
> Thanks for handling this) Also looking into this series I wonder why can't
> we instead of mangling ::it_signal zero bit just use ::it_id with negative
> value as a sign of not yet fully initialized timer? This would allow to not
> read-modify action while traversing bucket hash chain. I mean we could do
>
> static bool posix_timer_add_at(struct k_itimer *timer, struct signal_struct *sig, unsigned int id)
> {
> 	struct timer_hash_bucket *bucket = hash_bucket(sig, id);
>
> 	scoped_guard (spinlock, &bucket->lock) {
> 		if (!posix_timer_hashed(bucket, sig, id)) {
> --->			timer->it_id = -(timer_t)id;
> 			timer->it_signal = (struct signal_struct *)((unsigned long)sig | 1UL);
> 			hlist_add_head_rcu(&timer->t_hash, &bucket->head);
> 			return true;
> 		}
> 	}
> 	return false;
> }
>
> Then hash traverse won't find the timer until the do_timer_create will do
>
> 	scoped_guard (spinlock_irq, &current->sighand->siglock) {
> 		WRITE_ONCE(new_timer->it_id, abs(new_timer->it_id));
> 		hlist_add_head_rcu(&new_timer->list, &current->signal->posix_timers);
> 	}
>
> Or I miss something obvious? (Of course when deleting timer we will have to pass
> abs it_id for hash traversing).
>
> It looks that in case of many many timers present in the system traversing hash
> in read-modify way might be heavy (though I didn't measure of course).

The traversal does not RMW the timer itself, it unmangles the signal
pointer for comparison in posix_timer_hashed(). posix_timer_by_id() does
straight comparisons. So both only read.

Sure, we can mangle timer ID instead of the signal pointer, but the
outcome is pretty much the same. The only difference is in
posix_timer_hashed(), which must detect a taken timer ID independent of
the timers valid state to prevent collisions.

With the signal pointer mangling we have:

   if ((timer->signal & ~1) == sig && timer->id == id)

and with the negative ID value this becomes:

   if (timer->signal == sig && (timer->id == id || timer->id == -id))

which is obviously worse. You'd need to do:

      timer->id = id | (1 << 31);

and then the posix_timer_hashed() check becomes:

   if (timer->signal == sig && (timer->id & ~(1 << 31)) == id)

Granted, the timer ID mangling spares the AND operation on the signal in
case the timer is not owned by the currrent process, but I doubt that
this is even measurable.

Thanks,

        tglx