All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] alarmtimer: Fix bug where relative alarm timers were treated as absolute
@ 2014-07-07 21:06 John Stultz
  2014-07-08  6:42 ` Richard Cochran
  2014-07-08  8:52 ` [tip:timers/urgent] " tip-bot for John Stultz
  0 siblings, 2 replies; 4+ messages in thread
From: John Stultz @ 2014-07-07 21:06 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Thomas Gleixner, Ingo Molnar, Richard Cochran,
	Prarit Bhargava, Sharvil Nanavati, stable

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(-)

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;
 
-- 
1.9.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] alarmtimer: Fix bug where relative alarm timers were treated as absolute
  2014-07-07 21:06 [PATCH] alarmtimer: Fix bug where relative alarm timers were treated as absolute John Stultz
@ 2014-07-08  6:42 ` Richard Cochran
  2014-07-08  7:34   ` John Stultz
  2014-07-08  8:52 ` [tip:timers/urgent] " tip-bot for John Stultz
  1 sibling, 1 reply; 4+ messages in thread
From: Richard Cochran @ 2014-07-08  6:42 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Thomas Gleixner, Ingo Molnar, Prarit Bhargava,
	Sharvil Nanavati, stable

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] alarmtimer: Fix bug where relative alarm timers were treated as absolute
  2014-07-08  6:42 ` Richard Cochran
@ 2014-07-08  7:34   ` John Stultz
  0 siblings, 0 replies; 4+ messages in thread
From: John Stultz @ 2014-07-08  7:34 UTC (permalink / raw)
  To: Richard Cochran
  Cc: lkml, Thomas Gleixner, Ingo Molnar, Prarit Bhargava,
	Sharvil Nanavati, stable

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip:timers/urgent] alarmtimer: Fix bug where relative alarm timers were treated as absolute
  2014-07-07 21:06 [PATCH] alarmtimer: Fix bug where relative alarm timers were treated as absolute John Stultz
  2014-07-08  6:42 ` Richard Cochran
@ 2014-07-08  8:52 ` tip-bot for John Stultz
  1 sibling, 0 replies; 4+ messages in thread
From: tip-bot for John Stultz @ 2014-07-08  8:52 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, sharvil, john.stultz, hpa, mingo, tglx, prarit

Commit-ID:  16927776ae757d0d132bdbfabbfe2c498342bd59
Gitweb:     http://git.kernel.org/tip/16927776ae757d0d132bdbfabbfe2c498342bd59
Author:     John Stultz <john.stultz@linaro.org>
AuthorDate: Mon, 7 Jul 2014 14:06:11 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 8 Jul 2014 10:49:36 +0200

alarmtimer: Fix bug where relative alarm timers were treated as absolute

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.

Reported-by: Sharvil Nanavati <sharvil@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Sharvil Nanavati <sharvil@google.com>
Cc: stable <stable@vger.kernel.org> #3.0+
Link: http://lkml.kernel.org/r/1404767171-6902-1-git-send-email-john.stultz@linaro.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/alarmtimer.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

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;
 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-07-08  8:52 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-07 21:06 [PATCH] alarmtimer: Fix bug where relative alarm timers were treated as absolute John Stultz
2014-07-08  6:42 ` Richard Cochran
2014-07-08  7:34   ` John Stultz
2014-07-08  8:52 ` [tip:timers/urgent] " tip-bot for John Stultz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.