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

[Thomas Gleixner <tglx@linutronix.de>]

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