[PATCH 1/2] posix-timers: Report zero remaining time for disarmed SIGEV_NONE timers

[PATCH 1/2] posix-timers: Report zero remaining time for disarmed SIGEV_NONE timers

[Hung-Jui Lin]

SIGEV_NONE timers are never queued. common_timer_get() emulates their
expiry by computing the remaining time from the stored expiry value.
Because an armed SIGEV_NONE oneshot timer is not queued, its it_status
stays POSIX_TIMER_DISARMED. So common_timer_get() cannot use it_status to
skip a disarmed SIGEV_NONE timer and evaluates the stored expiry value for
all SIGEV_NONE timers.

Disarming a timer with timer_settime() and a zero it_value cancels the
timer and sets it_status to POSIX_TIMER_DISARMED, but leaves the expiry
value unchanged. As a consequence, timer_gettime() on a disarmed SIGEV_NONE
timer reports the remaining time of the previous arming until that time has
passed, instead of zero as POSIX requires for a disarmed timer. Example
with a CLOCK_MONOTONIC timer armed with it_value = 10ms and it_interval =
100ms, then disarmed with an all-zero itimerspec:

  armed:    it_value = 0.009999000  it_interval = 0.100000000
  disarmed: it_value = 0.008700792  it_interval = 0.000000000

SIGEV_SIGNAL timers and SIGEV_NONE CPU timers report zero in the same
situation. All clocks which use common_timer_get() are affected, including
the alarm timer clocks.

Mark SIGEV_NONE timers as POSIX_TIMER_ARMED when they are armed, so that
common_timer_get() returns early for every disarmed timer. Forward
SIGEV_NONE interval timers in common_timer_get() regardless of it_status,
as they are never queued and therefore never forwarded by the expiry code.

Tested on arm64 in QEMU: after the change, the disarmed timer above reports
zero. Armed SIGEV_NONE oneshot and interval timers on CLOCK_MONOTONIC,
CLOCK_REALTIME, CLOCK_BOOTTIME and CLOCK_BOOTTIME_ALARM still report their
remaining time, and the posix_timers selftest passes.

Fixes: becf8b5d00f4 ("[PATCH] hrtimer: convert posix timers completely")
Signed-off-by: Hung-Jui Lin <alex.lin.programmer@gmail.com>
---

Notes:
    Tested on arm64 under QEMU (virt, HVF), base tip timers/core
    348f54c435bf, arm64 defconfig plus kernel/configs/x86_debug.config
    (lockdep, DEBUG_OBJECTS, KMEMLEAK, DEBUG_PAGEALLOC, ...; the x86-only
    options do not exist on arm64), 9p built in:
    
                                            unpatched         patched
      reproducer: disarmed it_value         0.008700792       0
      posix_timers selftest, existing 19    19/19 pass        19/19 pass
      posix_timers selftest with patch 2    20/21, #14 fails  21/21 pass
    
    SIGEV_NONE timers on CLOCK_MONOTONIC, CLOCK_REALTIME, CLOCK_BOOTTIME,
    CLOCK_BOOTTIME_ALARM and CLOCK_PROCESS_CPUTIME_ID: never-armed, armed
    oneshot (5s), expired oneshot, armed interval and forwarded interval
    timers report the same values with and without the patch. Only the
    disarmed case changes: non-zero before (CPU timers were already 0),
    0 after. No lockdep or debugobjects splat after the tests. W=1 build of
    kernel/time/posix-timers.o has no warnings before or after.
    
    Fixes: tag checked by running the reproducer on x86_64 kernels in
    QEMU: 97735f25d2ba (the parent of becf8b5d00f4) reports 0 after
    disarm, becf8b5d00f4 reports the stale value. v2.6.17, v4.15 and v6.12
    also report the stale value.
    
    Not done: this patch was not built or booted on x86 or other
    architectures; no kmemleak scan was run; the series applies cleanly to
    mainline master a804c5742bff, but was not built or run there.

 kernel/time/posix-timers.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 188dbedbf..5eabec168 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -694,16 +694,8 @@ void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 	if (iv) {
 		cur_setting->it_interval = ktime_to_timespec64(iv);
 	} else if (timr->it_status == POSIX_TIMER_DISARMED) {
-		/*
-		 * SIGEV_NONE oneshot timers are never queued and therefore
-		 * timr->it_status is always DISARMED. The check below
-		 * vs. remaining time will handle this case.
-		 *
-		 * For all other timers there is nothing to update here, so
-		 * return.
-		 */
-		if (!sig_none)
-			return;
+		/* Nothing to update for a disarmed timer */
+		return;
 	}
 
 	now = kc->clock_get_ktime(timr->it_clock);
@@ -713,7 +705,7 @@ void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 	 * is a SIGEV_NONE timer move the expiry time forward by intervals,
 	 * so expiry is > now.
 	 */
-	if (iv && timr->it_status != POSIX_TIMER_ARMED)
+	if (iv && (sig_none || timr->it_status != POSIX_TIMER_ARMED))
 		timr->it_overrun += kc->timer_forward(timr, now);
 
 	remaining = kc->timer_remaining(timr, now);
@@ -915,8 +907,15 @@ int common_timer_set(struct k_itimer *timr, int flags,
 	sigev_none = timr->it_sigev_notify == SIGEV_NONE;
 
 	if (kc->timer_arm(timr, expires, flags & TIMER_ABSTIME, sigev_none)) {
-		if (!sigev_none)
-			timr->it_status = POSIX_TIMER_ARMED;
+		/*
+		 * SIGEV_NONE timers are not queued, but mark them armed
+		 * so that common_timer_get() can tell them apart from
+		 * disarmed timers. An expired SIGEV_NONE timer stays
+		 * armed, which is harmless: common_timer_get() reports
+		 * zero remaining time for it and it never reaches the
+		 * signal delivery code.
+		 */
+		timr->it_status = POSIX_TIMER_ARMED;
 	} else {
 		/* Timer was already expired, queue the signal */
 		posix_timer_queue_signal(timr);

base-commit: 348f54c435bf3177baa78201952810b255d44b30
-- 
2.56.0

[PATCH 2/2] selftests: timers: Add a test for disarmed SIGEV_NONE timers

[Hung-Jui Lin]

timer_gettime() must report a zero it_value for a disarmed timer. For
SIGEV_NONE timers, common_timer_get() reported the remaining time of the
previous arming instead.

Add check_sigev_none_disarm(). It arms a SIGEV_NONE oneshot timer, checks
that a remaining time is reported, disarms the timer and checks that
it_value is zero. Run it for CLOCK_MONOTONIC and CLOCK_PROCESS_CPUTIME_ID,
as CPU timers use a separate implementation.

Without the preceding fix, the CLOCK_MONOTONIC case fails.

Signed-off-by: Hung-Jui Lin <alex.lin.programmer@gmail.com>
---
 tools/testing/selftests/timers/posix_timers.c | 43 ++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/timers/posix_timers.c b/tools/testing/selftests/timers/posix_timers.c
index 52b7289e1..464609b8c 100644
--- a/tools/testing/selftests/timers/posix_timers.c
+++ b/tools/testing/selftests/timers/posix_timers.c
@@ -496,6 +496,45 @@ static void check_sigev_none(int which)
 			 "check_sigev_none %s\n", name);
 }
 
+static void check_sigev_none_disarm(int which)
+{
+	const char *name = clock_name(which);
+	struct itimerspec its;
+	struct sigevent sev;
+	timer_t timerid;
+	bool armed;
+
+	memset(&sev, 0, sizeof(sev));
+	sev.sigev_notify = SIGEV_NONE;
+
+	if (timer_create(which, &sev, &timerid))
+		fatal_error(name, "timer_create()");
+
+	/* Start a oneshot timer which does not expire during the test */
+	memset(&its, 0, sizeof(its));
+	its.it_value.tv_sec = 100;
+	if (timer_settime(timerid, 0, &its, NULL))
+		fatal_error(name, "timer_settime()");
+
+	if (timer_gettime(timerid, &its))
+		fatal_error(name, "timer_gettime()");
+	armed = its.it_value.tv_sec || its.it_value.tv_nsec;
+
+	/* Disarm it. The remaining time must be reported as 0 */
+	memset(&its, 0, sizeof(its));
+	if (timer_settime(timerid, 0, &its, NULL))
+		fatal_error(name, "timer_settime()");
+
+	if (timer_gettime(timerid, &its))
+		fatal_error(name, "timer_gettime()");
+
+	if (timer_delete(timerid))
+		fatal_error(name, "timer_delete()");
+
+	ksft_test_result(armed && !its.it_value.tv_sec && !its.it_value.tv_nsec,
+			 "%s %s\n", __func__, name);
+}
+
 static void check_gettime(int which)
 {
 	const char *name = clock_name(which);
@@ -681,7 +720,7 @@ int main(int argc, char **argv)
 
 	ksft_print_header();
 	if (run_sig_ign_tests) {
-		ksft_set_plan(19);
+		ksft_set_plan(21);
 	} else {
 		ksft_set_plan(10);
 	}
@@ -715,6 +754,8 @@ int main(int argc, char **argv)
 		check_delete();
 		check_sigev_none(CLOCK_MONOTONIC);
 		check_sigev_none(CLOCK_PROCESS_CPUTIME_ID);
+		check_sigev_none_disarm(CLOCK_MONOTONIC);
+		check_sigev_none_disarm(CLOCK_PROCESS_CPUTIME_ID);
 		check_gettime(CLOCK_MONOTONIC);
 		check_gettime(CLOCK_PROCESS_CPUTIME_ID);
 		check_gettime(CLOCK_THREAD_CPUTIME_ID);
-- 
2.56.0