[PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[Stanislaw Gruszka]

incr_error and error fields of struct cpu_itimer are used when calculating
next timer tick in check_cpu_itimers() and should not be modified without
tsk->sighand->siglock taken.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 kernel/itimer.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/itimer.c b/kernel/itimer.c
index b03451e..d802883 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -146,6 +146,7 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 {
 	cputime_t cval, nval, cinterval, ninterval;
 	s64 ns_ninterval, ns_nval;
+	u32 error, incr_error;
 	struct cpu_itimer *it = &tsk->signal->it[clock_id];
 
 	nval = timeval_to_cputime(&value->it_value);
@@ -153,8 +154,8 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 	ninterval = timeval_to_cputime(&value->it_interval);
 	ns_ninterval = timeval_to_ns(&value->it_interval);
 
-	it->incr_error = cputime_sub_ns(ninterval, ns_ninterval);
-	it->error = cputime_sub_ns(nval, ns_nval);
+	error = cputime_sub_ns(nval, ns_nval);
+	incr_error = cputime_sub_ns(ninterval, ns_ninterval);
 
 	spin_lock_irq(&tsk->sighand->siglock);
 
@@ -168,6 +169,8 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 	}
 	it->expires = nval;
 	it->incr = ninterval;
+	it->error = error;
+	it->incr_error = incr_error;
 	trace_itimer_state(clock_id == CPUCLOCK_VIRT ?
 			   ITIMER_VIRTUAL : ITIMER_PROF, value, nval);
 
-- 
1.6.2.5

[PATCH 2/2] posix-cpu-timers: initialize new_itimer->it.cpu.firing

[Stanislaw Gruszka]

In case when posix_cpu_timer_create() is called from sys_timer_create()
it.cpu.firing field of struct k_itimer is uninitialized.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 kernel/posix-cpu-timers.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 5c9dc22..e73a386 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -398,6 +398,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
 	INIT_LIST_HEAD(&new_timer->it.cpu.entry);
 	new_timer->it.cpu.incr.sched = 0;
 	new_timer->it.cpu.expires.sched = 0;
+	new_timer->it.cpu.firing = 0;
 
 	read_lock(&tasklist_lock);
 	if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
-- 
1.6.2.5

Re: [PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[Peter Zijlstra]

On Thu, 2009-09-24 at 16:35 +0200, Stanislaw Gruszka wrote:
> incr_error and error fields of struct cpu_itimer are used when calculating
> next timer tick in check_cpu_itimers() and should not be modified without
> tsk->sighand->siglock taken.

Won't it be all-round much better to convert these things to hrtimers
instead of adding more and more fuzz on top to make them deal with
jiffies?

Re: [PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[Stanislaw Gruszka]

On Thu, 24 Sep 2009 16:48:07 +0200
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Thu, 2009-09-24 at 16:35 +0200, Stanislaw Gruszka wrote:
> > incr_error and error fields of struct cpu_itimer are used when calculating
> > next timer tick in check_cpu_itimers() and should not be modified without
> > tsk->sighand->siglock taken.
> 
> Won't it be all-round much better to convert these things to hrtimers
> instead of adding more and more fuzz on top to make them deal with
> jiffies?

Perhaps it would, but I don't know how to do it :{ . Especially how to
precisely account user time. The only idea I have is make something like 
microstate accounting (http://lwn.net/Articles/127296/), but this patch
and whole idea was rejected long time ago. 

Stanislaw

Re: [PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[Peter Zijlstra]

On Thu, 2009-09-24 at 19:57 +0200, Stanislaw Gruszka wrote:
> On Thu, 24 Sep 2009 16:48:07 +0200
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Thu, 2009-09-24 at 16:35 +0200, Stanislaw Gruszka wrote:
> > > incr_error and error fields of struct cpu_itimer are used when calculating
> > > next timer tick in check_cpu_itimers() and should not be modified without
> > > tsk->sighand->siglock taken.
> > 
> > Won't it be all-round much better to convert these things to hrtimers
> > instead of adding more and more fuzz on top to make them deal with
> > jiffies?
> 
> Perhaps it would, but I don't know how to do it :{ . Especially how to
> precisely account user time. The only idea I have is make something like 
> microstate accounting (http://lwn.net/Articles/127296/), but this patch
> and whole idea was rejected long time ago. 

That patch does look a little painful indeed.

I was more thinking about about looking if an itimer was to expire less
than 1 tick away on either sched-in or the tick.

When we find it is indeed less than 1 tick away, program an hrtimer for
that cpu to expire at the required moment, see hrtick_start().

If we happen to de-schedule the task before the timer fires, we clear
the hrtimer again (or let it pend and ignore the fire), see
hrtick_clear().

[ there is no reason to rely on the tick though, we can program the
hrtimer on sched in to expire on at the right moment, and do so on each
schedule for as long as an itimer is active - re-setting whatever
pending timer the cpu still had. ]

Re: [PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Thu, 2009-09-24 at 19:57 +0200, Stanislaw Gruszka wrote:
> > On Thu, 24 Sep 2009 16:48:07 +0200
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > > On Thu, 2009-09-24 at 16:35 +0200, Stanislaw Gruszka wrote:
> > > > incr_error and error fields of struct cpu_itimer are used when calculating
> > > > next timer tick in check_cpu_itimers() and should not be modified without
> > > > tsk->sighand->siglock taken.
> > > 
> > > Won't it be all-round much better to convert these things to hrtimers
> > > instead of adding more and more fuzz on top to make them deal with
> > > jiffies?
> > 
> > Perhaps it would, but I don't know how to do it :{ . Especially how to
> > precisely account user time. The only idea I have is make something like 
> > microstate accounting (http://lwn.net/Articles/127296/), but this patch
> > and whole idea was rejected long time ago. 
> 
> That patch does look a little painful indeed.
> 
> I was more thinking about about looking if an itimer was to expire less
> than 1 tick away on either sched-in or the tick.
> 
> When we find it is indeed less than 1 tick away, program an hrtimer for
> that cpu to expire at the required moment, see hrtick_start().
> 
> If we happen to de-schedule the task before the timer fires, we clear
> the hrtimer again (or let it pend and ignore the fire), see
> hrtick_clear().
> 
> [ there is no reason to rely on the tick though, we can program the 
> hrtimer on sched in to expire on at the right moment, and do so on 
> each schedule for as long as an itimer is active - re-setting whatever 
> pending timer the cpu still had. ]

we should think about the simplest approach: switching itimers to 
hrtimers.

	Ingo

Re: [PATCH 1/2] itimers: fix racy writes to cpu_itimer fields

[Stanislaw Gruszka]

On Thu, Sep 24, 2009 at 10:19:43PM +0200, Ingo Molnar wrote:
> > [ there is no reason to rely on the tick though, we can program the 
> > hrtimer on sched in to expire on at the right moment, and do so on 
> > each schedule for as long as an itimer is active - re-setting whatever 
> > pending timer the cpu still had. ]
> 
> we should think about the simplest approach: switching itimers to 
> hrtimers.

I agree we need better implementation of posix-cpu-timers, however as
far we have this not perfect code and two patches fix bugs on it.
Not critical bugs, but I think they should be applied. Have I resend
them ?

Stanislaw

[tip:timers/urgent] itimers: Fix racy writes to cpu_itimer fields

[tip-bot for Stanislaw Gruszka]

Commit-ID:  8747d793fc5c4d3e4decd41d55f6dc24498dd5f5
Gitweb:     http://git.kernel.org/tip/8747d793fc5c4d3e4decd41d55f6dc24498dd5f5
Author:     Stanislaw Gruszka <sgruszka@redhat.com>
AuthorDate: Tue, 17 Nov 2009 14:14:12 -0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 18 Nov 2009 16:32:12 +0100

itimers: Fix racy writes to cpu_itimer fields

incr_error and error fields of struct cpu_itimer are used when calculating
next timer tick in check_cpu_itimers() and should not be modified without
tsk->sighand->siglock taken.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
LKML-Reference: <1253802903-979-1-git-send-email-sgruszka@redhat.com> 
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/itimer.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/itimer.c b/kernel/itimer.c
index b03451e..d802883 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -146,6 +146,7 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 {
 	cputime_t cval, nval, cinterval, ninterval;
 	s64 ns_ninterval, ns_nval;
+	u32 error, incr_error;
 	struct cpu_itimer *it = &tsk->signal->it[clock_id];
 
 	nval = timeval_to_cputime(&value->it_value);
@@ -153,8 +154,8 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 	ninterval = timeval_to_cputime(&value->it_interval);
 	ns_ninterval = timeval_to_ns(&value->it_interval);
 
-	it->incr_error = cputime_sub_ns(ninterval, ns_ninterval);
-	it->error = cputime_sub_ns(nval, ns_nval);
+	error = cputime_sub_ns(nval, ns_nval);
+	incr_error = cputime_sub_ns(ninterval, ns_ninterval);
 
 	spin_lock_irq(&tsk->sighand->siglock);
 
@@ -168,6 +169,8 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 	}
 	it->expires = nval;
 	it->incr = ninterval;
+	it->error = error;
+	it->incr_error = incr_error;
 	trace_itimer_state(clock_id == CPUCLOCK_VIRT ?
 			   ITIMER_VIRTUAL : ITIMER_PROF, value, nval);