Re: [PATCH 2/2] posix-timers: Use RCU in posix_timer_add()

From: Thomas Gleixner
Date: Tue Feb 18 2025 - 09:16:56 EST


On Mon, Feb 17 2025 at 20:24, Thomas Gleixner wrote:
> On Fri, Feb 14 2025 at 13:59, Eric Dumazet wrote:
>> @@ -112,7 +112,19 @@ static int posix_timer_add(struct k_itimer *timer)
>>
>> head = &posix_timers_hashtable[hash(sig, id)];
>>
>> + rcu_read_lock();
>> + if (__posix_timers_find(head, sig, id)) {
>> + rcu_read_unlock();
>> + cond_resched();
>> + continue;
>> + }
>> + rcu_read_unlock();
>> spin_lock(&hash_lock);
>> + /*
>> + * We must perform the lookup under hash_lock protection
>> + * because another thread could have used the same id.
>
> Hmm, that won't help and is broken already today as timer->id is set at
> the call site after releasing hash_lock.
>
>> + * This is very unlikely, but possible.
>
> Only if the process is able to install INT_MAX - 1 timers and the stupid
> search wraps around (INT_MAX loops) on the other thread and ends up at
> the same number again. But yes, theoretically it's possible. :)
>
> So the timer ID must be set _before_ adding it to the hash list, but
> that wants to be a seperate patch.

It's even worse. __posix_timers_find() checks for both timer->it_id and
timer->it_signal, but the latter is only set when the timer is about to
go live. I have an idea, but that might be a bad one :)

Thanks,

tglx