[PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Masami Hiramatsu]

Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure
that all kernel threads preempted on kprobe's boosted slot run out from
the slot even using freeze_processes().

The booster on preemptive kernel will be resumed if synchronize_tasks()
or something like that is introduced.

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jim Keniston <jkenisto@us.ibm.com>
CC: Mathieu Desnoyers <compudj@krystal.dyndns.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---

 arch/ia64/kernel/kprobes.c |    2 +-
 arch/x86/kernel/kprobes.c  |    2 +-
 kernel/kprobes.c           |   29 ++---------------------------
 3 files changed, 4 insertions(+), 29 deletions(-)

diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 9adac44..7026b29 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -870,7 +870,7 @@ static int __kprobes pre_kprobes_handler(struct die_args *args)
 		return 1;
 
 ss_probe:
-#if !defined(CONFIG_PREEMPT) || defined(CONFIG_FREEZER)
+#if !defined(CONFIG_PREEMPT)
 	if (p->ainsn.inst_flag == INST_FLAG_BOOSTABLE && !p->post_handler) {
 		/* Boost up -- we can execute copied instructions directly */
 		ia64_psr(regs)->ri = p->ainsn.slot;
diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index 5b8c750..9453815 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -429,7 +429,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 static void __kprobes setup_singlestep(struct kprobe *p, struct pt_regs *regs,
 				       struct kprobe_ctlblk *kcb)
 {
-#if !defined(CONFIG_PREEMPT) || defined(CONFIG_FREEZER)
+#if !defined(CONFIG_PREEMPT)
 	if (p->ainsn.boostable == 1 && !p->post_handler) {
 		/* Boost up -- we can execute copied instructions directly */
 		reset_current_kprobe();
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b7df302..9907a03 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -124,30 +124,6 @@ static LIST_HEAD(kprobe_insn_pages);
 static int kprobe_garbage_slots;
 static int collect_garbage_slots(void);
 
-static int __kprobes check_safety(void)
-{
-	int ret = 0;
-#if defined(CONFIG_PREEMPT) && defined(CONFIG_FREEZER)
-	ret = freeze_processes();
-	if (ret == 0) {
-		struct task_struct *p, *q;
-		do_each_thread(p, q) {
-			if (p != current && p->state == TASK_RUNNING &&
-			    p->pid != 0) {
-				printk("Check failed: %s is running\n",p->comm);
-				ret = -1;
-				goto loop_end;
-			}
-		} while_each_thread(p, q);
-	}
-loop_end:
-	thaw_processes();
-#else
-	synchronize_sched();
-#endif
-	return ret;
-}
-
 /**
  * __get_insn_slot() - Find a slot on an executable page for an instruction.
  * We allocate an executable page if there's no room on existing ones.
@@ -235,9 +211,8 @@ static int __kprobes collect_garbage_slots(void)
 {
 	struct kprobe_insn_page *kip, *next;
 
-	/* Ensure no-one is preepmted on the garbages */
-	if (check_safety())
-		return -EAGAIN;
+	/* Ensure no-one is interrupted on the garbages */
+	synchronize_sched();
 
 	list_for_each_entry_safe(kip, next, &kprobe_insn_pages, list) {
 		int i;


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Mathieu Desnoyers]

* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure
> that all kernel threads preempted on kprobe's boosted slot run out from
> the slot even using freeze_processes().
> 
> The booster on preemptive kernel will be resumed if synchronize_tasks()
> or something like that is introduced.


Yes, given that the freezer does not deal with non-freezable tasks (as
you pointed out in a different thread), I think we cannot rely on it
with CONFIG_PREEMPT.

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Jim Keniston <jkenisto@us.ibm.com>
> CC: Mathieu Desnoyers <compudj@krystal.dyndns.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
> 
>  arch/ia64/kernel/kprobes.c |    2 +-
>  arch/x86/kernel/kprobes.c  |    2 +-
>  kernel/kprobes.c           |   29 ++---------------------------
>  3 files changed, 4 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
> index 9adac44..7026b29 100644
> --- a/arch/ia64/kernel/kprobes.c
> +++ b/arch/ia64/kernel/kprobes.c
> @@ -870,7 +870,7 @@ static int __kprobes pre_kprobes_handler(struct die_args *args)
>  		return 1;
>  
>  ss_probe:
> -#if !defined(CONFIG_PREEMPT) || defined(CONFIG_FREEZER)
> +#if !defined(CONFIG_PREEMPT)
>  	if (p->ainsn.inst_flag == INST_FLAG_BOOSTABLE && !p->post_handler) {
>  		/* Boost up -- we can execute copied instructions directly */
>  		ia64_psr(regs)->ri = p->ainsn.slot;
> diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
> index 5b8c750..9453815 100644
> --- a/arch/x86/kernel/kprobes.c
> +++ b/arch/x86/kernel/kprobes.c
> @@ -429,7 +429,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  static void __kprobes setup_singlestep(struct kprobe *p, struct pt_regs *regs,
>  				       struct kprobe_ctlblk *kcb)
>  {
> -#if !defined(CONFIG_PREEMPT) || defined(CONFIG_FREEZER)
> +#if !defined(CONFIG_PREEMPT)
>  	if (p->ainsn.boostable == 1 && !p->post_handler) {
>  		/* Boost up -- we can execute copied instructions directly */
>  		reset_current_kprobe();
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index b7df302..9907a03 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -124,30 +124,6 @@ static LIST_HEAD(kprobe_insn_pages);
>  static int kprobe_garbage_slots;
>  static int collect_garbage_slots(void);
>  
> -static int __kprobes check_safety(void)
> -{
> -	int ret = 0;
> -#if defined(CONFIG_PREEMPT) && defined(CONFIG_FREEZER)
> -	ret = freeze_processes();
> -	if (ret == 0) {
> -		struct task_struct *p, *q;
> -		do_each_thread(p, q) {
> -			if (p != current && p->state == TASK_RUNNING &&
> -			    p->pid != 0) {
> -				printk("Check failed: %s is running\n",p->comm);
> -				ret = -1;
> -				goto loop_end;
> -			}
> -		} while_each_thread(p, q);
> -	}
> -loop_end:
> -	thaw_processes();
> -#else
> -	synchronize_sched();
> -#endif
> -	return ret;
> -}
> -
>  /**
>   * __get_insn_slot() - Find a slot on an executable page for an instruction.
>   * We allocate an executable page if there's no room on existing ones.
> @@ -235,9 +211,8 @@ static int __kprobes collect_garbage_slots(void)
>  {
>  	struct kprobe_insn_page *kip, *next;
>  
> -	/* Ensure no-one is preepmted on the garbages */
> -	if (check_safety())
> -		return -EAGAIN;
> +	/* Ensure no-one is interrupted on the garbages */
> +	synchronize_sched();
>  
>  	list_for_each_entry_safe(kip, next, &kprobe_insn_pages, list) {
>  		int i;
> 
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@redhat.com
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Ananth N Mavinakayanahalli]

On Wed, Jan 27, 2010 at 04:55:31PM -0500, Masami Hiramatsu wrote:
> Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure
> that all kernel threads preempted on kprobe's boosted slot run out from
> the slot even using freeze_processes().
> 
> The booster on preemptive kernel will be resumed if synchronize_tasks()
> or something like that is introduced.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure that 
> all kernel threads preempted on kprobe's boosted slot run out from the slot 
> even using freeze_processes().

hm, this really sucks as it makes preemptible kernels perform worse. Is there 
no better solution?

> The booster on preemptive kernel will be resumed if synchronize_tasks() or 
> something like that is introduced.

such as this one?

	Ingo

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Peter Zijlstra]

On Fri, 2010-01-29 at 10:21 +0100, Ingo Molnar wrote:
> * Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
> > Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure that 
> > all kernel threads preempted on kprobe's boosted slot run out from the slot 
> > even using freeze_processes().
> 
> hm, this really sucks as it makes preemptible kernels perform worse. Is there 
> no better solution?

We could pre-allocate 1 slot per kernel thread.

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Masami Hiramatsu]

Ingo Molnar wrote:
> 
> * Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
>> Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure that 
>> all kernel threads preempted on kprobe's boosted slot run out from the slot 
>> even using freeze_processes().
> 
> hm, this really sucks as it makes preemptible kernels perform worse. Is there 
> no better solution?
> 
>> The booster on preemptive kernel will be resumed if synchronize_tasks() or 
>> something like that is introduced.
> 
> such as this one?

Yes, I think this synchronize_tasks(), which just (sleeping) wait until
all currently preempted tasks are wake up and scheduled, can ensure safety.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Mathieu Desnoyers]

* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Ingo Molnar wrote:
> > 
> > * Masami Hiramatsu <mhiramat@redhat.com> wrote:
> > 
> >> Disable kprobe booster when CONFIG_PREEMPT=y, because it can't ensure that 
> >> all kernel threads preempted on kprobe's boosted slot run out from the slot 
> >> even using freeze_processes().
> > 
> > hm, this really sucks as it makes preemptible kernels perform worse. Is there 
> > no better solution?
> > 
> >> The booster on preemptive kernel will be resumed if synchronize_tasks() or 
> >> something like that is introduced.
> > 
> > such as this one?
> 
> Yes, I think this synchronize_tasks(), which just (sleeping) wait until
> all currently preempted tasks are wake up and scheduled, can ensure safety

If a task is set as stopped, and the preempted before calling schedule,
can this result in a preempted task staying in that state for an
arbitrary long period of time ? Or is there some mechanism prohibiting
that in the scheduler ?

Thanks,

Mathieu

> 
> Thank you,
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@redhat.com
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Peter Zijlstra]

On Fri, 2010-01-29 at 12:08 -0500, Mathieu Desnoyers wrote:
> 
> If a task is set as stopped, and the preempted before calling schedule,
> can this result in a preempted task staying in that state for an
> arbitrary long period of time ? Or is there some mechanism prohibiting
> that in the scheduler ? 

PREEMPT_ACTIVE does that:

preempt_schedule()
                add_preempt_count(PREEMPT_ACTIVE);
                schedule();


schedule()
        if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
                if (unlikely(signal_pending_state(prev->state, prev)))
                        prev->state = TASK_RUNNING;
                else
                        deactivate_task(rq, prev, 1);
                switch_count = &prev->nvcsw;
        }

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Mathieu Desnoyers]

* Peter Zijlstra (peterz@infradead.org) wrote:
> On Fri, 2010-01-29 at 12:08 -0500, Mathieu Desnoyers wrote:
> > 
> > If a task is set as stopped, and the preempted before calling schedule,
> > can this result in a preempted task staying in that state for an
> > arbitrary long period of time ? Or is there some mechanism prohibiting
> > that in the scheduler ? 
> 
> PREEMPT_ACTIVE does that:
> 
> preempt_schedule()
>                 add_preempt_count(PREEMPT_ACTIVE);
>                 schedule();
> 
> 
> schedule()
>         if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
>                 if (unlikely(signal_pending_state(prev->state, prev)))
>                         prev->state = TASK_RUNNING;
>                 else
>                         deactivate_task(rq, prev, 1);
>                 switch_count = &prev->nvcsw;
>         }

OK, it looks safe for preemption. Is there any unforeseen weird way a
task can be scheduled out and stopped that would permit it to either:

- stall the algorithm forever (DoS)
- appear as quiescent to the algorithm while its stack would hold return
  pointers to incorrect locations

?

I'm concerned about page faults here.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH tracing/kprobes] kprobes: Disable booster when CONFIG_PREEMPT=y

[Masami Hiramatsu]

Mathieu Desnoyers wrote:
> * Peter Zijlstra (peterz@infradead.org) wrote:
>> On Fri, 2010-01-29 at 12:08 -0500, Mathieu Desnoyers wrote:
>>>
>>> If a task is set as stopped, and the preempted before calling schedule,
>>> can this result in a preempted task staying in that state for an
>>> arbitrary long period of time ? Or is there some mechanism prohibiting
>>> that in the scheduler ? 
>>
>> PREEMPT_ACTIVE does that:
>>
>> preempt_schedule()
>>                 add_preempt_count(PREEMPT_ACTIVE);
>>                 schedule();
>>
>>
>> schedule()
>>         if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
>>                 if (unlikely(signal_pending_state(prev->state, prev)))
>>                         prev->state = TASK_RUNNING;
>>                 else
>>                         deactivate_task(rq, prev, 1);
>>                 switch_count = &prev->nvcsw;
>>         }
> 
> OK, it looks safe for preemption. Is there any unforeseen weird way a
> task can be scheduled out and stopped that would permit it to either:
> 
> - stall the algorithm forever (DoS)
> - appear as quiescent to the algorithm while its stack would hold return
>   pointers to incorrect locations
> 
> ?
> 
> I'm concerned about page faults here.

booster also checks whether the instruction can cause page-fault,
if it will cause it, kprobes doesn't boost it.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com