Commit-ID: eabdec04385376d560078992710104cc7be2ce1b
Gitweb: http://git.kernel.org/tip/eabdec04385376d560078992710104cc7be2ce1b
Author: Thomas Gleixner <[email protected]>
AuthorDate: Tue, 30 May 2017 23:15:51 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Sun, 4 Jun 2017 15:40:28 +0200
posix-timers: Zero settings value in common code
Zero out the settings struct in the common code so the callbacks do not
have to do it themself.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: John Stultz <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
---
kernel/time/posix-cpu-timers.c | 5 +----
kernel/time/posix-timers.c | 3 +--
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 96c833a..cb4a4eb 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -719,10 +719,8 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *itp
*/
itp->it_interval = ns_to_timespec64(timer->it.cpu.incr);
- if (timer->it.cpu.expires == 0) { /* Timer not armed at all. */
- itp->it_value.tv_sec = itp->it_value.tv_nsec = 0;
+ if (!timer->it.cpu.expires)
return;
- }
/*
* Sample the clock to take the difference with the expiry time.
@@ -746,7 +744,6 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *itp
* Call the timer disarmed, nothing else to do.
*/
timer->it.cpu.expires = 0;
- itp->it_value = ns_to_timespec64(timer->it.cpu.expires);
return;
} else {
cpu_timer_sample_group(timer->it_clock, p, &now);
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 48f6c37..0332f7a 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -645,8 +645,6 @@ common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
struct timespec64 ts64;
bool sig_none;
- memset(cur_setting, 0, sizeof(*cur_setting));
-
sig_none = (timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE;
iv = timr->it_interval;
@@ -705,6 +703,7 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id,
if (!timr)
return -EINVAL;
+ memset(&cur_setting64, 0, sizeof(cur_setting64));
kc = timr->kclock;
if (WARN_ON_ONCE(!kc || !kc->timer_get))
ret = -EINVAL;
Hello Thomas,
This patch breaks one of our CRIU tests:
https://github.com/xemul/criu/blob/master/test/zdtm/static/posix_timers.c#L145
python /root/git/main/criu/test/zdtm.py run -t zdtm/static/posix_timers --iter 0
====================== Run zdtm/static/posix_timers in h =======================
Start test
./posix_timers --pidfile=posix_timers.pid --outfile=posix_timers.out
Send the 15 signal to 24
Wait for zdtm/static/posix_timers(24) to die for 0.100000
############## Test zdtm/static/posix_timers FAIL at result check ##############
Test output: ================================
22:39:39.573: 24: ( start) boottime 12 boottime-coarse 12 total_sleep_time 0
22:39:39.602: 24: ( end) boottime 12 boottime-coarse 12 total_sleep_time 0
22:39:39.603: 24: FAIL: posix_timers.c:145: REALTIME (oneshot): timer became periodic
22:39:39.603: 24: FAIL: posix_timers.c:145: MONOTONIC (oneshot): timer became periodic
22:39:39.603: 24: FAIL: posix_timers.c:145: BOOTTIME (oneshot): timer became periodic
<<< ================================
=== Run 1/1 ================ zdtm/static/posix_timers
##################################### FAIL #####################################
This test sets a timer and then check that it is set correctly. And with this patch,
we see that timer_settime returns incorrect data.
2447 timer_settime(10, 0, {it_interval={tv_sec=0, tv_nsec=0}, it_value={tv_sec=2147483, tv_nsec=647000000}}, NULL) = 0
....
2519 timer_settime(10, 0, {it_interval={tv_sec=0, tv_nsec=0}, it_value={tv_sec=0, tv_nsec=0}}, <unfinished ...>
2509 <... exit_group resumed>) = ?
2519 <... timer_settime resumed> {it_interval={tv_sec=-1861316067, tv_nsec=18446645490300239488}, it_value={tv_sec=-1852565060, tv_nsec=133}}) = 0
Thanks,
Andrei
On Mon, Jun 05, 2017 at 01:21:46AM -0700, tip-bot for Jacob Shin wrote:
> Commit-ID: eabdec04385376d560078992710104cc7be2ce1b
> Gitweb: http://git.kernel.org/tip/eabdec04385376d560078992710104cc7be2ce1b
> Author: Thomas Gleixner <[email protected]>
> AuthorDate: Tue, 30 May 2017 23:15:51 +0200
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Sun, 4 Jun 2017 15:40:28 +0200
>
> posix-timers: Zero settings value in common code
>
> Zero out the settings struct in the common code so the callbacks do not
> have to do it themself.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: John Stultz <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> ---
> kernel/time/posix-cpu-timers.c | 5 +----
> kernel/time/posix-timers.c | 3 +--
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> index 96c833a..cb4a4eb 100644
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -719,10 +719,8 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *itp
> */
> itp->it_interval = ns_to_timespec64(timer->it.cpu.incr);
>
> - if (timer->it.cpu.expires == 0) { /* Timer not armed at all. */
> - itp->it_value.tv_sec = itp->it_value.tv_nsec = 0;
> + if (!timer->it.cpu.expires)
> return;
> - }
>
> /*
> * Sample the clock to take the difference with the expiry time.
> @@ -746,7 +744,6 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *itp
> * Call the timer disarmed, nothing else to do.
> */
> timer->it.cpu.expires = 0;
> - itp->it_value = ns_to_timespec64(timer->it.cpu.expires);
> return;
> } else {
> cpu_timer_sample_group(timer->it_clock, p, &now);
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 48f6c37..0332f7a 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -645,8 +645,6 @@ common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
> struct timespec64 ts64;
> bool sig_none;
>
> - memset(cur_setting, 0, sizeof(*cur_setting));
> -
> sig_none = (timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE;
> iv = timr->it_interval;
>
> @@ -705,6 +703,7 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id,
> if (!timr)
> return -EINVAL;
>
> + memset(&cur_setting64, 0, sizeof(cur_setting64));
> kc = timr->kclock;
> if (WARN_ON_ONCE(!kc || !kc->timer_get))
> ret = -EINVAL;
Commit-ID: 5c7a3a3d20a4e175304c0e23809e3d70be8fed8a
Gitweb: http://git.kernel.org/tip/5c7a3a3d20a4e175304c0e23809e3d70be8fed8a
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 12 Jun 2017 19:44:09 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Mon, 12 Jun 2017 21:07:40 +0200
posix-timers: Zero out oldval itimerspec
The recent posix timer rework moved the clearing of the itimerspec to the
real syscall implementation, but forgot that the kclock->timer_get() is
used by timer_settime() as well. That results in an uninitialized variable
and bogus values returned to user space.
Add the missing memset to timer_settime().
Fixes: eabdec043853 ("posix-timers: Zero settings value in common code")
Reported-by: Andrei Vagin <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Cyrill Gorcunov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
---
kernel/time/posix-timers.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index b53a0b5..88517dc 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -828,6 +828,8 @@ SYSCALL_DEFINE4(timer_settime, timer_t, timer_id, int, flags,
if (!timespec64_valid(&new_spec64.it_interval) ||
!timespec64_valid(&new_spec64.it_value))
return -EINVAL;
+ if (rtn)
+ memset(rtn, 0, sizeof(*rtn));
retry:
timr = lock_timer(timer_id, &flag);
if (!timr)
Commit-ID: 67edab48caeb75d412706f4b9d3107afd1e07623
Gitweb: http://git.kernel.org/tip/67edab48caeb75d412706f4b9d3107afd1e07623
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 12 Jun 2017 19:39:49 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Mon, 12 Jun 2017 21:07:41 +0200
posix-timers: Handle relative posix-timers correctly
The recent rework of the posix timer internals broke the magic posix
mechanism, which requires that relative timers are not affected by
modifications of the underlying clock. That means relative CLOCK_REALTIME
timers cannot use CLOCK_REALTIME, because that can be set and adjusted. The
underlying hrtimer switches the clock for these timers to CLOCK_MONOTONIC.
That still works, but reading the remaining time of such a timer has been
broken in the rework. The old code used the hrtimer internals directly and
avoided the posix clock callbacks. Now common_timer_get() uses the
underlying kclock->timer_get() callback, which is still CLOCK_REALTIME
based. So the remaining time of such a timer is calculated against the
wrong time base.
Handle it by switching the k_itimer->kclock pointer according to the
resulting hrtimer mode. k_itimer->it_clock still contains CLOCK_REALTIME
because the timer might be set with ABSTIME later and then it needs to
switch back to the realtime posix clock implementation.
Fixes: eae1c4ae275f ("posix-timers: Make use of cancel/arm callbacks")
Reported-by: Andrei Vagin <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Cyrill Gorcunov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
---
kernel/time/posix-timers.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 88517dc..58c0f60 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -72,6 +72,7 @@ static DEFINE_SPINLOCK(hash_lock);
static const struct k_clock * const posix_clocks[];
static const struct k_clock *clockid_to_kclock(const clockid_t id);
+static const struct k_clock clock_realtime, clock_monotonic;
/*
* we assume that the new SIGEV_THREAD_ID shares no bits with the other
@@ -750,6 +751,18 @@ static void common_hrtimer_arm(struct k_itimer *timr, ktime_t expires,
enum hrtimer_mode mode;
mode = absolute ? HRTIMER_MODE_ABS : HRTIMER_MODE_REL;
+ /*
+ * Posix magic: Relative CLOCK_REALTIME timers are not affected by
+ * clock modifications, so they become CLOCK_MONOTONIC based under the
+ * hood. See hrtimer_init(). Update timr->kclock, so the generic
+ * functions which use timr->kclock->clock_get() work.
+ *
+ * Note: it_clock stays unmodified, because the next timer_set() might
+ * use ABSTIME, so it needs to switch back.
+ */
+ if (timr->it_clock == CLOCK_REALTIME)
+ timr->kclock = absolute ? &clock_realtime : &clock_monotonic;
+
hrtimer_init(&timr->it.real.timer, timr->it_clock, mode);
timr->it.real.timer.function = posix_timer_fn;
On Mon, Jun 12, 2017 at 12:13:15PM -0700, tip-bot for Thomas Gleixner wrote:
> Commit-ID: 5c7a3a3d20a4e175304c0e23809e3d70be8fed8a
> Gitweb: http://git.kernel.org/tip/5c7a3a3d20a4e175304c0e23809e3d70be8fed8a
> Author: Thomas Gleixner <[email protected]>
> AuthorDate: Mon, 12 Jun 2017 19:44:09 +0200
> Committer: Thomas Gleixner <[email protected]>
> CommitDate: Mon, 12 Jun 2017 21:07:40 +0200
>
> posix-timers: Zero out oldval itimerspec
>
> The recent posix timer rework moved the clearing of the itimerspec to the
> real syscall implementation, but forgot that the kclock->timer_get() is
> used by timer_settime() as well. That results in an uninitialized variable
> and bogus values returned to user space.
>
> Add the missing memset to timer_settime().
>
> Fixes: eabdec043853 ("posix-timers: Zero settings value in common code")
> Reported-by: Andrei Vagin <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Cyrill Gorcunov <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> ---
> kernel/time/posix-timers.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index b53a0b5..88517dc 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -828,6 +828,8 @@ SYSCALL_DEFINE4(timer_settime, timer_t, timer_id, int, flags,
> if (!timespec64_valid(&new_spec64.it_interval) ||
> !timespec64_valid(&new_spec64.it_value))
> return -EINVAL;
> + if (rtn)
> + memset(rtn, 0, sizeof(*rtn));
Maybe we need to call memset after "retry:"?
common_timer_get() is called at the begining of common_timer_set(), then
common_timer_set() can return TIMER_RETRY. common_timer_get() will be
called again and some fields of rtn which have been touched first time
will not be touched.
At the end, rtn will contain data from two executions of
common_timer_get().
Thanks,
Andrei
> retry:
> timr = lock_timer(timer_id, &flag);
> if (!timr)
On Mon, 12 Jun 2017, Andrei Vagin wrote:
> > diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> > index b53a0b5..88517dc 100644
> > --- a/kernel/time/posix-timers.c
> > +++ b/kernel/time/posix-timers.c
> > @@ -828,6 +828,8 @@ SYSCALL_DEFINE4(timer_settime, timer_t, timer_id, int, flags,
> > if (!timespec64_valid(&new_spec64.it_interval) ||
> > !timespec64_valid(&new_spec64.it_value))
> > return -EINVAL;
> > + if (rtn)
> > + memset(rtn, 0, sizeof(*rtn));
>
> Maybe we need to call memset after "retry:"?
That would be counter productive.
> common_timer_get() is called at the begining of common_timer_set(), then
> common_timer_set() can return TIMER_RETRY. common_timer_get() will be
> called again and some fields of rtn which have been touched first time
> will not be touched.
>
> At the end, rtn will contain data from two executions of
> common_timer_get().
No. See the full code sequence:
retry:
timr = lock_timer(timer_id, &flag);
if (!timr)
return -EINVAL;
kc = clockid_to_kclock(timr->it_clock);
if (WARN_ON_ONCE(!kc || !kc->timer_set))
error = -EINVAL;
else
error = kc->timer_set(timr, flags, &new_spec64, rtn);
unlock_timer(timr, flag);
if (error == TIMER_RETRY) {
rtn = NULL; // We already got the old time...
goto retry;
}
If you clear it after retry, you'll get all zeros in the retry case. Not
what you really want.
Thanks,
tglx
On Tue, Jun 13, 2017 at 12:01:17AM +0200, Thomas Gleixner wrote:
> On Mon, 12 Jun 2017, Andrei Vagin wrote:
> > > diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> > > index b53a0b5..88517dc 100644
> > > --- a/kernel/time/posix-timers.c
> > > +++ b/kernel/time/posix-timers.c
> > > @@ -828,6 +828,8 @@ SYSCALL_DEFINE4(timer_settime, timer_t, timer_id, int, flags,
> > > if (!timespec64_valid(&new_spec64.it_interval) ||
> > > !timespec64_valid(&new_spec64.it_value))
> > > return -EINVAL;
> > > + if (rtn)
> > > + memset(rtn, 0, sizeof(*rtn));
> >
> > Maybe we need to call memset after "retry:"?
>
> That would be counter productive.
>
> > common_timer_get() is called at the begining of common_timer_set(), then
> > common_timer_set() can return TIMER_RETRY. common_timer_get() will be
> > called again and some fields of rtn which have been touched first time
> > will not be touched.
> >
> > At the end, rtn will contain data from two executions of
> > common_timer_get().
>
> No. See the full code sequence:
>
> retry:
> timr = lock_timer(timer_id, &flag);
> if (!timr)
> return -EINVAL;
>
> kc = clockid_to_kclock(timr->it_clock);
> if (WARN_ON_ONCE(!kc || !kc->timer_set))
> error = -EINVAL;
> else
> error = kc->timer_set(timr, flags, &new_spec64, rtn);
>
> unlock_timer(timr, flag);
> if (error == TIMER_RETRY) {
> rtn = NULL; // We already got the old time...
> goto retry;
> }
>
> If you clear it after retry, you'll get all zeros in the retry case. Not
> what you really want.
Yes, you are right. Sorry for the noise.
>
> Thanks,
>
> tglx