[PATCH v2 0/2] posix-timers: Prevents overrun counter overflow, adds DELAYTIMER_MAX

[PATCH v2 0/2] posix-timers: Prevents overrun counter overflow, adds DELAYTIMER_MAX

[Daniel Church]

If a timer overruns too many times before a call to timer_getoverrun the
overrun count can overflow and go negative.  POSIX.1-2001 specification
of timer_getoverrun() supports constant DELAYTIMER_MAX which prevents
overflow and caps overrun count.  Adds delaytimer_max value, caps
overrun count, and exposes value to userland via
/proc/sys/kernel/delaytimer_max.

Resolves https://bugzilla.kernel.org/show_bug.cgi?id=12665 - POSIX timers API does not support DELAYTIMER_MAX

v2: Consolidates overflow logic into helper function as suggested by Thomas Gleixner <tglx@linutronix.de>

[PATCH v2 1/2] posix-timers: Prevents overrun counter overflow

[Daniel Church]

If a timer overruns too many times before a call to timer_getoverrun the
overrun count can overflow and go negative.  Adds delaytimer_max value
to cap overrun count and prevent overflow.

Signed-off-by: Daniel Church <dchurch@andplus.com>
---
 include/linux/posix-timers.h |  3 +++
 kernel/time/posix-timers.c   | 45 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 907f3fd..dc8a1e7 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -138,4 +138,7 @@ long clock_nanosleep_restart(struct restart_block *restart_block);
 
 void update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
+#define DELAYTIMER_MAX_DEFAULT 1000000
+extern int delaytimer_max;
+
 #endif
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 31ea01f..010344e 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -68,6 +68,8 @@ static struct kmem_cache *posix_timers_cache;
 static DEFINE_HASHTABLE(posix_timers_hashtable, 9);
 static DEFINE_SPINLOCK(hash_lock);
 
+int delaytimer_max = DELAYTIMER_MAX_DEFAULT;
+
 /*
  * we assume that the new SIGEV_THREAD_ID shares no bits with the other
  * SIGEV values.  Here we put out an error if this assumption fails.
@@ -202,6 +204,24 @@ static inline void unlock_timer(struct k_itimer *timr, unsigned long flags)
 	spin_unlock_irqrestore(&timr->it_lock, flags);
 }
 
+/*
+ * Updates a timer's overrun count while capping it to delaytimer_max
+ */
+static void posix_timer_update_overrun_count(struct k_itimer *timer,
+					     unsigned int overruns)
+{
+	const bool newOverrunsAboveMax = overruns >= delaytimer_max;
+	const bool totalOverrunsAboveMax =
+		timer->it_overrun >= 0 &&
+		timer->it_overrun >= delaytimer_max - overruns;
+
+	if (newOverrunsAboveMax || totalOverrunsAboveMax) {
+		timer->it_overrun = delaytimer_max;
+	} else {
+		timer->it_overrun += overruns;
+	}
+}
+
 /* Get clock_realtime */
 static int posix_clock_realtime_get(clockid_t which_clock, struct timespec *tp)
 {
@@ -350,14 +370,17 @@ __initcall(init_posix_timers);
 
 static void schedule_next_timer(struct k_itimer *timr)
 {
+	unsigned int overruns;
 	struct hrtimer *timer = &timr->it.real.timer;
 
 	if (timr->it.real.interval.tv64 == 0)
 		return;
 
-	timr->it_overrun += (unsigned int) hrtimer_forward(timer,
-						timer->base->get_time(),
-						timr->it.real.interval);
+	overruns = (unsigned int) hrtimer_forward(timer,
+					timer->base->get_time(),
+					timr->it.real.interval);
+
+	posix_timer_update_overrun_count(timr, overruns);
 
 	timr->it_overrun_last = timr->it_overrun;
 	timr->it_overrun = -1;
@@ -436,6 +459,7 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 {
 	struct k_itimer *timr;
 	unsigned long flags;
+	unsigned int overruns;
 	int si_private = 0;
 	enum hrtimer_restart ret = HRTIMER_NORESTART;
 
@@ -484,9 +508,10 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 					now = ktime_add(now, kj);
 			}
 #endif
-			timr->it_overrun += (unsigned int)
-				hrtimer_forward(timer, now,
-						timr->it.real.interval);
+			overruns = (unsigned int) hrtimer_forward(timer, now,
+								 timr->it.real.interval);
+			posix_timer_update_overrun_count(timr, overruns);
+
 			ret = HRTIMER_RESTART;
 			++timr->it_requeue_pending;
 		}
@@ -729,6 +754,7 @@ static void
 common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
 {
 	ktime_t now, remaining, iv;
+	unsigned int overruns;
 	struct hrtimer *timer = &timr->it.real.timer;
 
 	memset(cur_setting, 0, sizeof(struct itimerspec));
@@ -750,8 +776,10 @@ common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
 	 * expiry is > now.
 	 */
 	if (iv.tv64 && (timr->it_requeue_pending & REQUEUE_PENDING ||
-	    (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE))
-		timr->it_overrun += (unsigned int) hrtimer_forward(timer, now, iv);
+	    (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE)) {
+		overruns = (unsigned int) hrtimer_forward(timer, now, iv);
+		posix_timer_update_overrun_count(timr, overruns);
+	}
 
 	remaining = ktime_sub(hrtimer_get_expires(timer), now);
 	/* Return 0 only, when the timer is expired and not pending */
@@ -1122,3 +1150,4 @@ long clock_nanosleep_restart(struct restart_block *restart_block)
 
 	return kc->nsleep_restart(restart_block);
 }
+
-- 
1.9.1

Re: [PATCH v2 1/2] posix-timers: Prevents overrun counter overflow

[Richard Cochran]

On Sat, Jan 24, 2015 at 12:48:56PM -0500, Daniel Church wrote:
> +/*
> + * Updates a timer's overrun count while capping it to delaytimer_max
> + */
> +static void posix_timer_update_overrun_count(struct k_itimer *timer,
> +					     unsigned int overruns)
> +{
> +	const bool newOverrunsAboveMax = overruns >= delaytimer_max;
> +	const bool totalOverrunsAboveMax =
> +		timer->it_overrun >= 0 &&
> +		timer->it_overrun >= delaytimer_max - overruns;

Lower camel case is not a part of the kernel coding style.

Also, the 'const' keyword is useless in this context.

> +
> +	if (newOverrunsAboveMax || totalOverrunsAboveMax) {
> +		timer->it_overrun = delaytimer_max;
> +	} else {
> +		timer->it_overrun += overruns;
> +	}
> +}
> +
>  /* Get clock_realtime */
>  static int posix_clock_realtime_get(clockid_t which_clock, struct timespec *tp)
>  {
> @@ -1122,3 +1150,4 @@ long clock_nanosleep_restart(struct restart_block *restart_block)
>  
>  	return kc->nsleep_restart(restart_block);
>  }
> +

This stray newline and the camel case are the kinds of things that
checkpatch.pl will catch.

Thanks,
Richard

Re: [PATCH v2 1/2] posix-timers: Prevents overrun counter overflow

[Thomas Gleixner]

On Sat, 24 Jan 2015, Daniel Church wrote:
> +/*
> + * Updates a timer's overrun count while capping it to delaytimer_max
> + */
> +static void posix_timer_update_overrun_count(struct k_itimer *timer,
> +					     unsigned int overruns)
> +{
> +	const bool newOverrunsAboveMax = overruns >= delaytimer_max;
> +	const bool totalOverrunsAboveMax =
> +		timer->it_overrun >= 0 &&
> +		timer->it_overrun >= delaytimer_max - overruns;

No CaMelCaSe please. And the const here is pointless.

Aside of that in a function like this we really want short local
variables so we can avoid the horrible to read multi line code.

> +	if (newOverrunsAboveMax || totalOverrunsAboveMax) {
> +		timer->it_overrun = delaytimer_max;
> +	} else {
> +		timer->it_overrun += overruns;
> +	}
> +}
> +
>  /* Get clock_realtime */
>  static int posix_clock_realtime_get(clockid_t which_clock, struct timespec *tp)
>  {
> @@ -350,14 +370,17 @@ __initcall(init_posix_timers);
>  
>  static void schedule_next_timer(struct k_itimer *timr)
>  {
> +	unsigned int overruns;
>  	struct hrtimer *timer = &timr->it.real.timer;
>  
>  	if (timr->it.real.interval.tv64 == 0)
>  		return;
>  
> -	timr->it_overrun += (unsigned int) hrtimer_forward(timer,
> -						timer->base->get_time(),
> -						timr->it.real.interval);
> +	overruns = (unsigned int) hrtimer_forward(timer,
> +					timer->base->get_time(),
> +					timr->it.real.interval);

Why not:

  posix_timer_forward(struct hrtimer *hrt, struct k_itimer *tmr)      			     
			     
and doing the forward there as well? The now optimization in
common_timer_get is not that important and if we really want to keep
it, we can hand in a pointer and read the time in the function if the
pointer is NULL.

Thanks,

	tglx

Re: [PATCH v2 1/2] posix-timers: Prevents overrun counter overflow

[Carlos O'Donell]

On 01/24/2015 12:48 PM, Daniel Church wrote:
> +#define DELAYTIMER_MAX_DEFAULT 1000000

Why did you chose 1 million?

Have you reviewed what constant userspace, particularly glibc, is using?

Cheers,
Carlos.

[PATCH v2 2/2] posix-timers: Exposes DELAYTIMER_MAX constant used to govern overruns

[Daniel Church]

POSIX.1-2001 specification of timer_getoverrun() supports constant
DELAYTIMER_MAX which prevents overflow and caps overrun count.  Exposes
delaytimer_max value to userland via /proc/sys/kernel/delaytimer_max such
that GLIBC can support DELAYTIMER_MAX constant.

Signed-off-by: Daniel Church <dchurch@andplus.com>
---
 kernel/sysctl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 137c7f6..b283808 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -63,6 +63,7 @@
 #include <linux/binfmts.h>
 #include <linux/sched/sysctl.h>
 #include <linux/kexec.h>
+#include <linux/posix-timers.h>
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
@@ -761,6 +762,13 @@ static struct ctl_table kern_table[] = {
 	},
 #endif
 	{
+		.procname	= "delaytimer_max",
+		.data		= &delaytimer_max,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
+	{
 		.procname	= "pid_max",
 		.data		= &pid_max,
 		.maxlen		= sizeof (int),
-- 
1.9.1

Re: [PATCH v2 2/2] posix-timers: Exposes DELAYTIMER_MAX constant used to govern overruns

[Thomas Gleixner]

On Sat, 24 Jan 2015, Daniel Church wrote:

> POSIX.1-2001 specification of timer_getoverrun() supports constant
> DELAYTIMER_MAX which prevents overflow and caps overrun count.  Exposes
> delaytimer_max value to userland via /proc/sys/kernel/delaytimer_max such

I know that you try to match the posix name, but when looking at the
prctl w/o knowing about DELAYTIMER_MAX, it's non intuitive.

Something like posixtimer_max_overruns or if you insist on the
delaytimer_max part, then let us prepend it with posixtimer_ at least.

New prctls require an update of Documentation/sysctl/kernel.txt.

Thanks,

	tglx

Re: [PATCH v2 2/2] posix-timers: Exposes DELAYTIMER_MAX constant used to govern overruns

[Carlos O'Donell]

On 01/24/2015 02:20 PM, Thomas Gleixner wrote:
> On Sat, 24 Jan 2015, Daniel Church wrote:
> 
>> POSIX.1-2001 specification of timer_getoverrun() supports constant
>> DELAYTIMER_MAX which prevents overflow and caps overrun count.  Exposes
>> delaytimer_max value to userland via /proc/sys/kernel/delaytimer_max such
> 
> I know that you try to match the posix name, but when looking at the
> prctl w/o knowing about DELAYTIMER_MAX, it's non intuitive.
> 
> Something like posixtimer_max_overruns or if you insist on the
> delaytimer_max part, then let us prepend it with posixtimer_ at least.
> 
> New prctls require an update of Documentation/sysctl/kernel.txt.

Agreed, the name should be self-explanatory and point at POSIX.

The documentation should be detailed and explain exactly what this
is for, what are valid values, and why you would ever want to change
them.

Cheers,
Carlos.