Re: [PATCH] kprobe-booster: boosting multi-probe

["bibo,mao" <bibo dot mao at intel dot com>]

Masami Hiramatsu wrote:
> Hi, bibo
>
> bibo,mao wrote:
> > Hi Marami,
> > I refresh kprobe boost patch as follows, in this patch post_handler is
> > seperated from break_handler.
>
> The part of the patch against kernel/kprobes.c seems good to me.
> But I found a bug.
>
> > @@ -537,6 +539,18 @@ valid_p:
> >          }
> >          arch_remove_kprobe(p);
> >      }
> > +    else{
> > +        mutex_lock(&kprobe_mutex);
> > +        if (p->break_handler)
> > +            old_p->break_handler = NULL;
> > +        if (p->post_handler){
> > +            list_for_each_entry_rcu(list_p, &old_p->list, list){
> > +                if (list_p->post_handler) break;
> > +                old_p->post_handler = NULL;
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> When the head of the list does not have a post_handler, this code
> always clear the aggr_kprobe's post_handler.
I have modified it.

> And the next part is not comfortable to me.
>
>
> I think your patch enables booster even when preemption is
> enabled, and it may be dangerous. Some running processes can
> be preempted by another process when it is executing the codes in
> the instruction buffer. When the boosted-probe is deregistered, that
> instruction buffer is removed. Then, those preempted processes
> can't continue to run, because they can't back to the preempted address.
> So, I think, for the safety, the booster should NOT be enabled when
> preemption is enabled.
> Please fix it.
It actually is one problem, now I fix it. But actually most time kprobe 
happen in preempt enable places, such as system call entry position, 
then booster function will lose its effect.



 arch/i386/kernel/kprobes.c |   14 ++------------
 kernel/kprobes.c           |   34 ++++++++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 20 deletions(-)
--- 2.6.16-rc6-mm1.org/kernel/kprobes.c	2006-03-13 12:25:18.000000000 +0800
+++ 2.6.16-rc6-mm1/kernel/kprobes.c	2006-03-14 08:56:01.000000000 +0800
@@ -368,16 +368,15 @@ static inline void copy_kprobe(struct kp
 */
 static int __kprobes add_new_kprobe(struct kprobe *old_p, struct 
kprobe *p)
 {
-        struct kprobe *kp;
-
 	if (p->break_handler) {
-		list_for_each_entry_rcu(kp, &old_p->list, list) {
-			if (kp->break_handler)
-				return -EEXIST;
-		}
+		if (old_p->break_handler)
+			return -EEXIST;
 		list_add_tail_rcu(&p->list, &old_p->list);
+		old_p->break_handler = aggr_break_handler;
 	} else
 		list_add_rcu(&p->list, &old_p->list);
+	if (p->post_handler && !old_p->post_handler)
+		old_p->post_handler = aggr_post_handler;
 	return 0;
 }

@@ -390,9 +389,12 @@ static inline void add_aggr_kprobe(struc
 	copy_kprobe(p, ap);
 	ap->addr = p->addr;
 	ap->pre_handler = aggr_pre_handler;
-	ap->post_handler = aggr_post_handler;
 	ap->fault_handler = aggr_fault_handler;
-	ap->break_handler = aggr_break_handler;
+	if (p->post_handler)
+		ap->post_handler = aggr_post_handler;
+	if (p->break_handler)
+		ap->break_handler = aggr_break_handler;
+

 	INIT_LIST_HEAD(&ap->list);
 	list_add_rcu(&p->list, &ap->list);
@@ -537,6 +539,22 @@ valid_p:
 		}
 		arch_remove_kprobe(p);
 	}
+	else{
+		mutex_lock(&kprobe_mutex);
+		if (p->break_handler)
+			old_p->break_handler = NULL;
+		if (p->post_handler){
+			list_for_each_entry_rcu(list_p, &old_p->list, list){
+				if (list_p->post_handler){
+					cleanup_p = 2;
+					 break;
+				}
+			}
+			if (cleanup_p == 0)
+				old_p->post_handler = NULL;
+		}
+		mutex_unlock(&kprobe_mutex);
+	}
 }

 static struct notifier_block kprobe_exceptions_nb = {
--- 2.6.16-rc6-mm1.org/arch/i386/kernel/kprobes.c	2006-03-13 
12:41:01.000000000 +0800
+++ 2.6.16-rc6-mm1/arch/i386/kernel/kprobes.c	2006-03-14 
09:27:56.000000000 +0800
@@ -205,9 +205,7 @@ static int __kprobes kprobe_handler(stru
 	int ret = 0;
 	kprobe_opcode_t *addr;
 	struct kprobe_ctlblk *kcb;
-#ifdef CONFIG_PREEMPT
 	unsigned pre_preempt_count = preempt_count();
-#endif /* CONFIG_PREEMPT */

 	addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));

@@ -293,22 +291,14 @@ static int __kprobes kprobe_handler(stru
 		/* handler has already set things up, so skip ss setup */
 		return 1;

-	if (p->ainsn.boostable == 1 &&
-#ifdef CONFIG_PREEMPT
-	    !(pre_preempt_count) && /*
-				       * This enables booster when the direct
-				       * execution path aren't preempted.
-				       */
-#endif /* CONFIG_PREEMPT */
-	    !p->post_handler && !p->break_handler ) {
+ss_probe:
+	if (pre_preempt_count && p->ainsn.boostable == 1 && !p->post_handler) {
 		/* Boost up -- we can execute copied instructions directly */
 		reset_current_kprobe();
 		regs->eip = (unsigned long)p->ainsn.insn;
 		preempt_enable_no_resched();
 		return 1;
 	}
-
-ss_probe:
 	prepare_singlestep(p, regs);
 	kcb->kprobe_status = KPROBE_HIT_SS;
 	return 1;