[RFC][PATCH 0/2] Fix for settimeofday() error checking regression

[RFC][PATCH 0/2] Fix for settimeofday() error checking regression

[John Stultz]

As reported by Mika Westerberg here:
	https://lkml.org/lkml/2016/5/30/413

A regression in the settimeofday error checking snuck in via
86d3473224b0 ("time: Introduce do_sys_settimeofday64()"),
effecting cases where the timeval is null but the timezone
was set.

This patchset contains my proposed fix and an addition to
the kselftests to add checks for this case.

Testing/feedback would be greatly appreciated!

thanks
-john

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>


John Stultz (2):
  time: Fix problematic change in settimeofday error checking
  kselftests: timers: Add set-tz test case

 include/linux/timekeeping.h             |   3 +
 tools/testing/selftests/timers/Makefile |   3 +-
 tools/testing/selftests/timers/set-tz.c | 119 ++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/timers/set-tz.c

-- 
1.9.1

[RFC][PATCH 1/2] time: Fix problematic change in settimeofday error checking

[John Stultz]

In commit 86d3473224b0 ("time: Introduce do_sys_settimeofday64()")
some of the checking for a valid timeval was subtley changed
which caused -EINVAL to be returned whenever the timeval was null.

However, it is possible to set the timezone data while specifying
a NULL timeval, which is usually done to handle systems where the
RTC keeps local time instead of UTC. Thus the patch causes such
systems to have the time incorrectly set.

This patch addresses the issue by handling the error conditionals
in the same way as was done previously.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/timekeeping.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 37dbacf..4f01607 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -21,6 +21,9 @@ static inline int do_sys_settimeofday(const struct timespec *tv,
 	struct timespec64 ts64;
 
 	if (!tv)
+		return do_sys_settimeofday64(NULL, tz);
+
+	if (tv && !timespec_valid(tv))
 		return -EINVAL;
 
 	ts64 = timespec_to_timespec64(*tv);
-- 
1.9.1

[RFC][PATCH 2/2] kselftests: timers: Add set-tz test case

[John Stultz]

Mika Westerberg reported a erroneous change in the error
checking of settimeofday, so I wanted to add a test to ensure
we don't trip over this again.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 tools/testing/selftests/timers/Makefile |   3 +-
 tools/testing/selftests/timers/set-tz.c | 119 ++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/timers/set-tz.c

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 4a1be1b..1d55568 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile
@@ -10,7 +10,7 @@ TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
 
 TEST_PROGS_EXTENDED = alarmtimer-suspend valid-adjtimex adjtick change_skew \
 		      skew_consistency clocksource-switch leap-a-day \
-		      leapcrash set-tai set-2038
+		      leapcrash set-tai set-2038 set-tz
 
 bins = $(TEST_PROGS) $(TEST_PROGS_EXTENDED)
 
@@ -30,6 +30,7 @@ run_destructive_tests: run_tests
 	./clocksource-switch
 	./leap-a-day -s -i 10
 	./leapcrash
+	./set-tz
 	./set-tai
 	./set-2038
 
diff --git a/tools/testing/selftests/timers/set-tz.c b/tools/testing/selftests/timers/set-tz.c
new file mode 100644
index 0000000..f418492
--- /dev/null
+++ b/tools/testing/selftests/timers/set-tz.c
@@ -0,0 +1,119 @@
+/* Set tz value
+ *              by: John Stultz <john.stultz@linaro.org>
+ *              (C) Copyright Linaro 2016
+ *              Licensed under the GPLv2
+ *
+ *   This program is free software: you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation, either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+#include <sys/time.h>
+#include <sys/timex.h>
+#include <string.h>
+#include <signal.h>
+#include <unistd.h>
+#ifdef KTEST
+#include "../kselftest.h"
+#else
+static inline int ksft_exit_pass(void)
+{
+	exit(0);
+}
+static inline int ksft_exit_fail(void)
+{
+	exit(1);
+}
+#endif
+
+int set_tz(int min, int dst)
+{
+	struct timezone tz;
+
+	tz.tz_minuteswest = min;
+	tz.tz_dsttime = dst;
+
+	return settimeofday(0, &tz);
+}
+
+int get_tz_min(void)
+{
+	struct timezone tz;
+	struct timeval tv;
+
+	memset(&tz, 0, sizeof(tz));
+	gettimeofday(&tv, &tz);
+	return tz.tz_minuteswest;
+}
+
+int get_tz_dst(void)
+{
+	struct timezone tz;
+	struct timeval tv;
+
+	memset(&tz, 0, sizeof(tz));
+	gettimeofday(&tv, &tz);
+	return tz.tz_dsttime;
+}
+
+int main(int argc, char **argv)
+{
+	int i, ret;
+	int min, dst;
+
+	min = get_tz_min();
+	dst = get_tz_dst();
+	printf("tz_minuteswest started at %i, dst at %i\n", min, dst);
+
+	printf("Checking tz_minuteswest can be properly set: ");
+	for (i = -15*60; i < 15*60; i += 30) {
+		ret = set_tz(i, dst);
+		ret = get_tz_min();
+		if (ret != i) {
+			printf("[FAILED] expected: %i got %i\n", i, ret);
+			goto err;
+		}
+	}
+	printf("[OK]\n");
+
+	printf("Checking invalid tz_minuteswest values are caught: ");
+
+	if (!set_tz(-15*60-1, dst)) {
+		printf("[FAILED] %i didn't return failure!\n", -15*60-1);
+		goto err;
+	}
+
+	if (!set_tz(15*60+1, dst)) {
+		printf("[FAILED] %i didn't return failure!\n", 15*60+1);
+		goto err;
+	}
+
+	if (!set_tz(-24*60, dst)) {
+		printf("[FAILED] %i didn't return failure!\n", -24*60);
+		goto err;
+	}
+
+	if (!set_tz(24*60, dst)) {
+		printf("[FAILED] %i didn't return failure!\n", 24*60);
+		goto err;
+	}
+
+	printf("[OK]\n");
+
+	set_tz(min, dst);
+	return ksft_exit_pass();
+
+err:
+	set_tz(min, dst);
+	return ksft_exit_fail();
+}
-- 
1.9.1

Re: [RFC][PATCH 1/2] time: Fix problematic change in settimeofday error checking

[Dima Stepanov]

> @@ -21,6 +21,9 @@ static inline int do_sys_settimeofday(const struct timespec *tv,
>         struct timespec64 ts64;
>
>         if (!tv)
> +               return do_sys_settimeofday64(NULL, tz);
> +
> +       if (tv && !timespec_valid(tv))
>                 return -EINVAL;

Looks like an extra check for (tv), maybe it will be better to use:
  +       if (!timespec_valid(tv))

Regards, Dima.

Re: [RFC][PATCH 1/2] time: Fix problematic change in settimeofday error checking

[Mika Westerberg]

On Tue, May 31, 2016 at 02:32:14PM -0700, John Stultz wrote:
> In commit 86d3473224b0 ("time: Introduce do_sys_settimeofday64()")
> some of the checking for a valid timeval was subtley changed
> which caused -EINVAL to be returned whenever the timeval was null.
> 
> However, it is possible to set the timezone data while specifying
> a NULL timeval, which is usually done to handle systems where the
> RTC keeps local time instead of UTC. Thus the patch causes such
> systems to have the time incorrectly set.
> 
> This patch addresses the issue by handling the error conditionals
> in the same way as was done previously.
> 
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Baolin Wang <baolin.wang@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I can confirm that this patch fixes the issue I'm seeing. Thanks!

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>