Hi Andi,

I have updated the kprobes patch with you suggestions.
Please find patch below.

Your comments are welcome.

Thanks
Prasanna

On Thu, Aug 05, 2004 at 12:55:17PM +0200, Andi Kleen wrote:
> Prasanna S Panchamukhi <prasanna@in.ibm.com> writes:
> 
> Hallo,
> 
> Overall it looks good, but some issues noted below. 
> 
> >  		goto gp_in_kernel;
> >  
> > @@ -557,6 +560,17 @@ void unset_nmi_callback(void)
> >  	nmi_callback = dummy_nmi_callback;
> >  }
> >  
> > +asmlinkage int do_int3(struct pt_regs *regs, long error_code)
> > +{
> > +	if (kprobe_handler(regs))
> > +		return 1;
> 
> Same.
> 
> > +	/* This is an interrupt gate, because kprobes wants interrupts
> > +           disabled.  Normal trap handlers don't. */
> > +	restore_interrupts(regs);
> > +	do_trap(3, SIGTRAP, "int3", 1, regs, error_code, NULL);
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Our handling of the processor debug registers is non-trivial.
> >   * We do not clear them on entry and exit from the kernel. Therefore
> > @@ -579,12 +593,15 @@ void unset_nmi_callback(void)
> >   * find every occurrence of the TF bit that could be saved away even
> >   * by user code)
> >   */
> > -asmlinkage void do_debug(struct pt_regs * regs, long error_code)
> > +asmlinkage int do_debug(struct pt_regs * regs, long error_code)
> >  {
> >  	unsigned int condition;
> >  	struct task_struct *tsk = current;
> >  	siginfo_t info;
> >  
> > +	if (post_kprobe_handler(regs))
> > +		return 1;
> > +
> 
> Also notifier list.
> 
> >  clear_TF_reenable:
> >  	set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
> >  clear_TF:
> >  	regs->eflags &= ~TF_MASK;
> > -	return;
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -817,6 +834,8 @@ asmlinkage void math_state_restore(struc
> >  	struct thread_info *thread = current_thread_info();
> >  	struct task_struct *tsk = thread->task;
> >  
> > +	if (kprobe_running() && kprobe_fault_handler(&regs, 7))
> > +		return;
> 
> Same.
> 
> 
> >  	clts();		/* Allow maths ops (or we recurse) */
> >  	if (!tsk->used_math)
> >  		init_fpu(tsk);
> > @@ -911,7 +930,7 @@ void __init trap_init(void)
> >  	set_trap_gate(0,&divide_error);
> >  	set_intr_gate(1,&debug);
> >  	set_intr_gate(2,&nmi);
> > -	set_system_gate(3,&int3);	/* int3-5 can be called from all */
> > +	_set_gate(idt_table+3,14,3,&int3,__KERNEL_CS); /* int3-5 can be called from all */
> 
> Please define a macro for this (set_system_intr_gate) 
> 
> 
> > +#include <linux/kprobes.h>
> >  
> >  #include <asm/system.h>
> >  #include <asm/uaccess.h>
> > @@ -226,6 +227,9 @@ asmlinkage void do_page_fault(struct pt_
> >  	/* get the address */
> >  	__asm__("movl %%cr2,%0":"=r" (address));
> >  
> > +	if (kprobe_running() && kprobe_fault_handler(regs, 14))
> > +		return;
> 
> Also notifier here. 
> 
> > +/* trap3/1 are intr gates for kprobes.  So, restore the status of IF,
> > + * if necessary, before executing the original int3/1 (trap) handler.
> > + */
> > +static inline void restore_interrupts(struct pt_regs *regs)
> > +{
> > +	if (regs->eflags & IF_MASK)
> > +		__asm__ __volatile__ ("sti");
> 
> That's local_irq_enable()
> 
> > +
> > +/* You have to be holding the kprobe_lock */
> > +struct kprobe *get_kprobe(void *addr)
> > +{
> > +	struct list_head *head, *tmp;
> 
> A hlist would be probably better for the hash table (saves some space) 
> 
> > +
> > +int register_kprobe(struct kprobe *p)
> > +{
> > +	int ret = 0;
> > +
> > +	spin_lock_irq(&kprobe_lock);
> 
> Better use _irqsave here. Safer interface for users.
> 
> 
> > +
> > +void unregister_kprobe(struct kprobe *p)
> > +{
> > +	spin_lock_irq(&kprobe_lock);
> 
> Better make this an _irqsave() 
> 
> > +	*p->addr = p->opcode;
> > +	list_del(&p->list);
> > +	flush_icache_range(p->addr, p->addr + sizeof(kprobe_opcode_t));
> 
> This flush will probably need good review by other architecture
> maintainers. it's ok right now since it is 386 only.
> My suspection is that it is will not be enough for some people
> 
> 
> -Andi
> 

-- 

Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<prasanna@in.ibm.com>