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: <ZxpM0NLZc_MAGoNc@localhost.localdomain>
Date: Thu, 24 Oct 2024 15:34:08 +0200
From: Frederic Weisbecker <frederic@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
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

Le Thu, Oct 24, 2024 at 11:10:09AM +0200, Thomas Gleixner a écrit :
> On Thu, Oct 24 2024 at 10:57, Thomas Gleixner wrote:
> > 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.
> 
> The signal will be delivered. It's either already marked as pending, but
> not yet delivered or it is masked and will be marked pending on unmask.

Oh I see now, si_sys_private is still overwritten with the new seq value
even if the signal is already queued. So yes the old queued signal will
remain valid and apply for the latest event. Phew!

> 
> >>> +
> >>>  	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.
> 
> Of course it can happen that the signal is queued. See above scenario :)
> 
> But q->info.si_overrun should never be anything else than 0. The timer code
> increments the overrun counter and updates si_overrun on signal
> delivery, but that's done on the siginfo which is a copy of q->info.

Bah right of course. But then, there is an undocumented side effect
to this patch in that it also resets the value when a queued signal
is ignored.

> 
> This seems to be a leftover from early posix-timer days... So that
> change can be reduced to removing the increment.

Ok. One thing you can add to this patch is the following, since now si_overrun
is expected to be 0 upon delivery (nothing can change it between the
enqueue and the delivery)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index ff7970030b17..0d44e3d87cd3 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -233,11 +233,12 @@ __initcall(init_posix_timers);
  * The siginfo si_overrun field and the return value of timer_getoverrun(2)
  * are of type int. Clamp the overrun value to INT_MAX
  */
-static inline int timer_overrun_to_int(struct k_itimer *timr, int baseval)
+static inline int timer_overrun_to_int(struct k_itimer *timr)
 {
-	s64 sum = timr->it_overrun_last + (s64)baseval;
+	if (timr->it_overrun_last > (s64)INT_MAX)
+		return INT_MAX;
 
-	return sum > (s64)INT_MAX ? INT_MAX : (int)sum;
+	return (int)timr->it_overrun_last;
 }
 
 static void common_hrtimer_rearm(struct k_itimer *timr)
@@ -280,7 +281,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 		timr->it_overrun = -1LL;
 		++timr->it_signal_seq;
 
-		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
+		info->si_overrun = timer_overrun_to_int(timr);
 	}
 	ret = true;
 
@@ -783,7 +784,7 @@ SYSCALL_DEFINE1(timer_getoverrun, timer_t, timer_id)
 	if (!timr)
 		return -EINVAL;
 
-	overrun = timer_overrun_to_int(timr, 0);
+	overrun = timer_overrun_to_int(timr);
 	unlock_timer(timr, flags);
 
 	return overrun;