Re: [Fwd: Re: [PATCH] Return probe]

[Ananth N Mavinakayanahalli <amavin at redhat dot com>]

Hien Nguyen wrote:
> Hi Prasanna,
> Here's the new retprobe patch, this patch should apply to 
> linix-2.6.12-rc2-mm2 + Anandth's multihadler (the one posted on LKML).
>
> Thanks for your suggestions.

Hien,

Some comments...

Thanks,
Ananth

...

> +struct kprobe trampoline_p = {
> +		.addr = (kprobe_opcode_t *) &kretprobe_trampoline,
> +		.pre_handler = trampoline_probe_handler,
> +		.post_handler = trampoline_post_handler
> +};
> +
> +struct kretprobe_instance * get_free_rp_inst(struct kretprobe *rp)
> +{
> +	struct hlist_node *node;
> +	struct kretprobe_instance *ri;
> +	hlist_for_each_entry(ri, node, &rp->free_instances, uflist)
> +		return ri;
> +	return NULL;
> +}	
> +
> +struct kretprobe_instance * get_used_rp_inst(struct kretprobe *rp)

Please make this consistent everywhere.. kretprobe_instance *get_used_..

....

> +struct kretprobe_instance *get_rp_inst_tsk(struct task_struct *tk)
> +{
> +	struct task_struct *tsk;
> +	struct hlist_head *head;
> +	struct hlist_node *node;
> +	struct kretprobe_instance *ri;
> +
> +	head = &kretprobe_inst_table[hash_ptr(tk, KPROBE_HASH_BITS)];
> +	

Whitespace mangling.. here and a lot of other places.

> +	hlist_for_each_entry(ri, node, head, hlist) {
> +		tsk = arch_get_kprobe_task(ri->stack_addr);
> +		if (tsk == tk)
> +			return ri;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * This function is called from do_exit or do_execv when task tk's stack is
> + * about to be recycled. Recycle any function-return probe instances 
> + * associated with this task. These represent probed functions that have 
> + * been called but may never return.
> + */
> +void kprobe_flush_task(struct task_struct *tk)
> +{
> +	unsigned long flags = 0;
> +	struct kretprobe_instance *ri;
> +	
> +	if (!arch_supports_kretprobes)
> +		return;
> +		
> +	spin_lock_irqsave(&kprobe_lock, flags);
> +	while ((ri = get_rp_inst_tsk(tk)) != NULL) {
> +		/* TODO: arch specific */
> +		*((unsigned long *)(ri->stack_addr)) = (unsigned long) ri->ret_addr;

Coding style (limit to 80 chars) here and a few other places

> +		recycle_rp_inst(ri);
> +	}	
> +	spin_unlock_irqrestore(&kprobe_lock, flags);	
> +}
> +
> +/*
> + * This kprobe pre_handler is registered with every kretprobe. When probe
> + * hits it will set up the return probe.
> + */
> +int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> +{
> +	struct kretprobe *rp = container_of(p, struct kretprobe, kp);
> +
> +	/*TODO: consider to only swap the RA after the last pre_handler fired */
> +	arch_prepare_kretprobe(rp, regs);
> +	return 0;
> +}
> +
> +inline void free_rp_inst(struct kretprobe *rp)
> +{
> +	struct kretprobe_instance *ri;
> +	while((ri = get_free_rp_inst(rp)) != NULL) {

Coding style - while ( not while(

> +		hlist_del(&ri->uflist);
> +		kfree(ri);
> +	}
> +}
> +
>  /* 
>   * Fill in the required fields of the "manager kprobe". Replace the 
>   * earlier kprobe in the hlist with the manager kprobe
> @@ -260,16 +402,75 @@
>  	unregister_kprobe(&jp->kp);
>  }
>  
> +int register_kretprobe(struct kretprobe *rp)
> +{
> +	int ret = 0;
> +	struct kretprobe_instance *inst;
> +	int i;
> +	
> +	if (!arch_supports_kretprobes)
> +		return -ENOSYS;
> +	
> +	rp->kp.pre_handler = pre_handler_kretprobe;
> +	
> +	/* Pre-allocate memory for max kretprobe instances */
> +	if (rp->maxactive <= 0) {
> +#ifdef CONFIG_PREEMPT
> +		rp->maxactive = max(10, 2 * NR_CPUS);
> +#else
> +		rp->maxactive = NR_CPUS;
> +#endif
> +	} 
> +	INIT_HLIST_HEAD(&rp->used_instances);
> +	INIT_HLIST_HEAD(&rp->free_instances);
> +	for (i = 0; i < rp->maxactive; i++) {
> +		inst = kmalloc(sizeof(struct kretprobe_instance), GFP_KERNEL);
> +		if (inst == NULL) {
> +			free_rp_inst(rp);
> +			return -ENOMEM;
> +		}
> +		INIT_HLIST_NODE(&inst->uflist);
> +		hlist_add_head(&inst->uflist, &rp->free_instances);
> +	}
> +	
> +	rp->nmissed = 0;
> +	/* Establish function entry probe point */
> +	if ((ret = register_kprobe(&rp->kp)) != 0) {
> +		free_rp_inst(rp);
> +	}

Coding style... no braces for single line conditionals

....

> +struct task_struct  *arch_get_kprobe_task(void *ptr)
> +{
> +	return ((struct thread_info *) (((unsigned long) ptr) & (~(THREAD_SIZE -1))))->task;

80 columns.. split after the unary/logical operator

...

> + * Called when we hit the probe point at kretprobe_trampoline
> + */  
> +int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
> +{
> +	struct task_struct *tsk;
> +	struct kretprobe_instance *ri;
> +	struct hlist_head *head;
> +	struct hlist_node *node;
> +	unsigned long *sara = ((unsigned long *) &regs->esp) - 1;
> +	
> +	tsk = arch_get_kprobe_task(sara);
> +	head = kretprobe_inst_table_head(tsk);
> +
> +	hlist_for_each_entry(ri, node, head, hlist)
> +		if (ri->stack_addr == sara && ri->rp) 
> +			ri->rp->handler(ri, regs);

Unconditional call - what if the user did not define a ->handler?