Re: [(resend)patch 7/7] Kprobes - Register for page fault notify on active probes

[Keshavamurthy Anil S <anil.s.keshavamurthy@intel.com>]

On Thu, Apr 20, 2006 at 09:27:35AM +0530, Ananth N Mavinakayanahalli wrote:
> On Wed, Apr 19, 2006 at 03:14:26PM -0700, Anil S Keshavamurthy wrote:
> > With this patch Kprobes now registers for page fault notifications only
> > when their is an active probe registered. Once all the active probes are
> > unregistered their is no need to be notified of page faults and kprobes
> > unregisters itself from the page fault notifications. Hence we
> > will have ZERO side effects when no probes are active.
> 
> This patch isn't complete yet... comments below.
>  
> > Signed-off-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> > ---
> >  kernel/kprobes.c |   25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > Index: linux-2.6.17-rc1-mm3/kernel/kprobes.c
> > ===================================================================
> > --- linux-2.6.17-rc1-mm3.orig/kernel/kprobes.c
> > +++ linux-2.6.17-rc1-mm3/kernel/kprobes.c
> > @@ -47,11 +47,17 @@
> >  
> >  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> >  static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
> > +static atomic_t kprobe_count;
> >  
> >  DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
> >  DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
> >  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
> >  
> > +static struct notifier_block kprobe_page_fault_nb = {
> > +	.notifier_call = kprobe_exceptions_notify,
> > +	.priority = 0x7fffffff /* we need to notified first */
> > +};
> > +
> >  #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
> >  /*
> >   * kprobe->ainsn.insn points to the copy of the instruction to be
> > @@ -464,6 +470,8 @@ static int __kprobes __register_kprobe(s
> >  	old_p = get_kprobe(p->addr);
> >  	if (old_p) {
> >  		ret = register_aggr_kprobe(old_p, p);
> > +		if (!ret)
> > +			atomic_inc(&kprobe_count);
> >  		goto out;
> >  	}
> >  
> > @@ -474,6 +482,9 @@ static int __kprobes __register_kprobe(s
> >  	hlist_add_head_rcu(&p->hlist,
> >  		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
> >  
> > +	if(atomic_add_return(1, &kprobe_count) == 2)
> 	^^^
> 	if (..) please, here and elsewhere
> 
> This will not work as desired for i386 (i386 no longer has a kprobe on the
> trampoline) and sparc64 (no retprobe support).
Instead of hardcoding value 2, #define KPROBE_ARCH_INITIAL_COUNT x
should do the trick.
> 
> > +		register_page_fault_notifier(&kprobe_page_fault_nb);
> > +
> >    	arch_arm_kprobe(p);
> >  
> >  out:
> > @@ -523,9 +534,13 @@ valid_p:
> >  		cleanup_p = 0;
> >  	}
> >  
> > +	if(atomic_add_return(-1, &kprobe_count) == 1)
> > +		unregister_page_fault_notifier(&kprobe_page_fault_nb);
> 
> Same here - i386 and sparc64 need different checks.
Yup, will take care in my next version.

> 
> > +	else
> > +		synchronize_rcu();
> 
> As of now, synchronize_sched() is aliased to synchronize_rcu() but I am
> told its scheduled to change in the future.
> 
> Please revert this back to synchronize_sched().
> 
I will revert it back in my take2 :)

Thanks again for your valuable feedback.

-Anil Keshavamurthy