| Message ID | 1404767171-6902-1-git-send-email-john.stultz@linaro.org |
|---|---|
| State | Accepted |
| Commit | 16927776ae757d0d132bdbfabbfe2c498342bd59 |
| Headers | show |
On Mon, Jul 07, 2014 at 02:06:11PM -0700, John Stultz wrote: > @@ -597,8 +602,16 @@ static int alarm_timer_set(struct k_itimer *timr, int flags, > > /* start the timer */ > timr->it.alarm.interval = timespec_to_ktime(new_setting->it_interval); > - alarm_start(&timr->it.alarm.alarmtimer, > - timespec_to_ktime(new_setting->it_value)); > + exp = timespec_to_ktime(new_setting->it_value); > + /* Convert (if necessary) to absolute time */ > + if (flags != TIMER_ABSTIME) { > + ktime_t now; > + > + now = alarm_bases[timr->it.alarm.alarmtimer.type].gettime(); > + exp = ktime_add(now, exp); > + } > + > + alarm_start(&timr->it.alarm.alarmtimer, exp); Nothing protects 'exp' from becoming invalid before queuing the alarm, if the time base is reset on another cpu. Or would that be harmless here? > return 0; > } Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Jul 8, 2014 at 8:42 AM, Richard Cochran <richardcochran@gmail.com> wrote: > On Mon, Jul 07, 2014 at 02:06:11PM -0700, John Stultz wrote: >> @@ -597,8 +602,16 @@ static int alarm_timer_set(struct k_itimer *timr, int flags, >> >> /* start the timer */ >> timr->it.alarm.interval = timespec_to_ktime(new_setting->it_interval); >> - alarm_start(&timr->it.alarm.alarmtimer, >> - timespec_to_ktime(new_setting->it_value)); >> + exp = timespec_to_ktime(new_setting->it_value); >> + /* Convert (if necessary) to absolute time */ >> + if (flags != TIMER_ABSTIME) { >> + ktime_t now; >> + >> + now = alarm_bases[timr->it.alarm.alarmtimer.type].gettime(); >> + exp = ktime_add(now, exp); >> + } >> + >> + alarm_start(&timr->it.alarm.alarmtimer, exp); > > Nothing protects 'exp' from becoming invalid before queuing the alarm, > if the time base is reset on another cpu. Or would that be harmless > here? Hrmn.. So that's a separate question, but a good one to validate as well. common_timer_set() has a similar behavior where the return value of hrtimer_start_expires() isn't passed up. This is because the userspace behavior is that if the time has already passed, the timer should fire immediately. If you look in __hrtimer_start_range_ns(), you'll see the chunk of logic at the bottom which (if I'm reading it right) raises the softirq (to fire the timer) if our timer is the earliest and enqueue_reprogram fails (due to the clockevent logic returning ETIME due to the time being in the past). So its definitely subtle but looks like its ok. But I'll add a validation test to be sure I'm not missing anything there as well. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index 88c9c65..fe75444 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -585,9 +585,14 @@ static int alarm_timer_set(struct k_itimer *timr, int flags, struct itimerspec *new_setting, struct itimerspec *old_setting) { + ktime_t exp; + if (!rtcdev) return -ENOTSUPP; + if (flags & ~TIMER_ABSTIME) + return -EINVAL; + if (old_setting) alarm_timer_get(timr, old_setting); @@ -597,8 +602,16 @@ static int alarm_timer_set(struct k_itimer *timr, int flags, /* start the timer */ timr->it.alarm.interval = timespec_to_ktime(new_setting->it_interval); - alarm_start(&timr->it.alarm.alarmtimer, - timespec_to_ktime(new_setting->it_value)); + exp = timespec_to_ktime(new_setting->it_value); + /* Convert (if necessary) to absolute time */ + if (flags != TIMER_ABSTIME) { + ktime_t now; + + now = alarm_bases[timr->it.alarm.alarmtimer.type].gettime(); + exp = ktime_add(now, exp); + } + + alarm_start(&timr->it.alarm.alarmtimer, exp); return 0; } @@ -730,6 +743,9 @@ static int alarm_timer_nsleep(const clockid_t which_clock, int flags, if (!alarmtimer_get_rtcdev()) return -ENOTSUPP; + if (flags & ~TIMER_ABSTIME) + return -EINVAL; + if (!capable(CAP_WAKE_ALARM)) return -EPERM;
Sharvil noticed with the posix timer_settime interface, using the CLOCK_REALTIME_ALARM or CLOCK_BOOTTIME_ALARM clockid, if the users tried to specify a relative time timer, it would incorrectly be treated as absolute regardless of the state of the flags argument. This patch corrects this, properly checking the absolute/relative flag, as well as adds further error checking that no invalid flag bits are set. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@kernel.org> Cc: Richard Cochran <richardcochran@gmail.com> Cc: Prarit Bhargava <prarit@redhat.com> Cc: Sharvil Nanavati <sharvil@google.com> Cc: stable <stable@vger.kernel.org> #3.0+ Reported-by: Sharvil Nanavati <sharvil@google.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- Thomas, Ingo: Please consider for tip/timers/urgent for 3.16 kernel/time/alarmtimer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)