linux-kernel - Re: [patch V5 09/26] posix-timers: Make signal overrun accounting sensible

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: <87ldyd6fq8.ffs@tglx>
Date: Thu, 24 Oct 2024 10:57:03 +0200
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>, John Stultz <jstultz@...gle.com>, Peter
 Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, Stephen
 Boyd <sboyd@...nel.org>, Eric Biederman <ebiederm@...ssion.com>, Oleg
 Nesterov <oleg@...hat.com>
Subject: Re: [patch V5 09/26] posix-timers: Make signal overrun accounting
 sensible

On Mon, Oct 21 2024 at 23:54, Frederic Weisbecker wrote:
> Le Tue, Oct 01, 2024 at 10:42:12AM +0200, Thomas Gleixner a écrit :
>> +	/*
>> +	 * Set the overrun count to zero unconditionally. The posix timer
>> +	 * code does not self rearm periodic timers. They are rearmed from
>> +	 * dequeue_signal().
>> +	 *
>> +	 * But there is a situation where @q is already enqueued:
>> +	 *
>> +	 * 1) timer_settime()
>> +	 *      arm_timer()
>> +	 * 2) timer_expires()
>> +	 *      send_sigqueue(@q)
>> +	 *        enqueue(@q)
>> +	 * 3) timer_settime()
>> +	 *      arm_timer()
>> +	 * 4) timer_expires()
>> +	 *      send_sigqueue(@q) <- Observes @q already queued
>> +	 *
>> +	 * In this case incrementing si_overrun does not make sense because
>> +	 * there is no relationship between timer_settime() #1 and #2.
>> +	 *
>> +	 * The POSIX specification is useful as always: "The effect of
>> +	 * disarming or resetting a timer with pending expiration
>> +	 * notifications is unspecified."
>> +	 *
>> +	 * Just do the sensible thing and reset the overrun.
>> +	 */
>> +	q->info.si_overrun = 0;
>
> So this means that in the above example case, no signal at all is going to be
> delivered (because the seq will be impaired on the previously queued
> signal) and no overrun count will be incremented either?

So #2 queues the signal, but before delivery the timer is rearmed, which
invalidates the signal via the sequence count. So #4 has to set the
overrun counter which might be set already.

>> +
>>  	ret = 1; /* the signal is ignored */
>>  	result = TRACE_SIGNAL_IGNORED;
>>  	if (!prepare_signal(sig, t, false))
>> @@ -1968,15 +1996,9 @@ int send_sigqueue(struct sigqueue *q, st
>>  
>>  	ret = 0;
>>  	if (unlikely(!list_empty(&q->list))) {
>> -		/*
>> -		 * If an SI_TIMER entry is already queue just increment
>> -		 * the overrun count.
>> -		 */
>> -		q->info.si_overrun++;
>
> Who is ever incrementing this after that? I'm a bit confused between the
> timer overrun and the sigqueue overrun. Those seem to be two different
> things without any link...

Hmm. You're right. This should now never happen. Let me stare at it some
moar.

Thanks,

        tglx