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

[Hien Nguyen <hien at us dot ibm dot com>]

Hi Prasanna,

See my replies in line.

Prasanna S Panchamukhi wrote:

> +
> +struct kretprobe_instance {
> +	struct list_head list;
> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^why do you need this list? You can use 
> 	one hlist_node and add it to free_instaces if free or to the 
> 	kretprobe_inst_table_head[] if kretprobe_instance is not free.
> +	struct hlist_node hlist;
>  
Yes, actually Jim suggested this sometime ago too somehow I managed to 
forget/overlooked.  I made that change today.

> 		
> 	
> +	struct kretprobe *rp;
> +	void *ret_addr;
> 		^^^^^^^^^^^^^^^^^^^^^^assign it to NULL, when on free list and 
> 		assign address when in use. You can use this logic and find out
> 		if the instance is on the free list or on the 
> 		kretprobe_inst_table[] during unregistering the kretprobe and 
> 		remove the instances.
> +	void *stack_addr;
> +};
>
> +struct kretprobe {
>
> +	int num_ri_running;
> 	^^^^^^^^^^^^^^^^^^^
> +	int unregistering;
> 	^^^^^^^^^^^^^^^^^^^^^^^^^Are you trying to use these flags to remove 
> 	the instances while unregistering?
>  

Yes, these are to keep track of number of instances are still pending 
(the return address has been replaced with the trampoline on stack) 
during unregistering.

> +#define RPROBE_HASH_BITS	KPROBE_HASH_BITS	
> +#define RPROBE_INST_TABLE_SIZE  KPROBE_TABLE_SIZE
> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^you can also use the same 
> 	KRPOBE_HASH_BITS unless you use a different size and hash bits.
> +
> +
>  
Got that changed.

> +struct kretprobe_instance * get_free_rp_inst(struct kretprobe *rp)
> +{
> +	if (list_empty(&rp->free_instances)) {
> +		return NULL;
> +	}
> +	return (struct kretprobe_instance *) rp->free_instances.next;
> +}	
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 		here you can get the next entry using existing macro 
> 		something like this.
>            hlist_for_each_entry(ri, node, &rp->free_instances, hlist)
> 		return ri;
>
> +void recycle_kretprobe_instance(struct kretprobe_instance *ri)
> +{
> +	ri->rp->num_ri_running--;
> +	if (ri->rp->num_ri_running == 0 && ri->rp->unregistering == 1) {
> +		/* This is the last running ri during unregister. 
> +		 * Free memory to complete the unregister.
> +		 */
>
> +		kfree(ri->rp->instances);
> +		kfree(ri->rp);
> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^can be done at 
> 			unregister_kretprobe()..see below.
> +		tsk = arch_get_kprobe_task(ri->stack_addr);
> +		if (tsk == tk) {
> +			/* Put the original return address back into stack */
> +			*((unsigned long *)(ri->stack_addr)) = (unsigned long) ri->ret_addr;
> +			hlist_del_rcu(&ri->hlist);
> +			recycle_kretprobe_instance(ri);
> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^you need just remove ri from the 
> 	kretprobe_inst_table[] and add it to the free_instances.
>
> +int register_kretprobe(struct kretprobe *rp)
> +{
> +	int ret = 0;
> +	struct kretprobe_instance *inst;
> +	int maxinst, i;
> ^^^^^^^^^^^^^^^^^^^^^^maxinst can be avoided here.
> +	
> +	/* Pre-allocate memory for max kretprobe instances */
> +#ifdef CONFIG_PREEMPT
> +		maxinst = max(10, 2 * NR_CPUS);
> 		^^^^^^^^^use rp->maxactive.
> +#else
> +		maxinst = NR_CPUS;
> 		^^^^^^^^^^^^same as above.
> +#endif
> +	} 
> +	rp->instances = kmalloc(maxinst * sizeof(struct kretprobe_instance), 
> +					GFP_KERNEL);
> +	if (rp->instances == NULL) {
> +		return -ENOMEM;
> +	}
> +	
> +	INIT_LIST_HEAD(&rp->free_instances);
> +	/* Put all kretprobe_instance objects on the free list */
> +	for (i = 0; i < maxinst; i++) {
> +		inst = rp->instances + i;
> +		list_add(&inst->list, &rp->free_instances);
> +	}
> 	^^^^^^^^^^^^^^^^^^^^^^^^register first,if success then do all the above.
> 	use hlist to save some space.
> +	rp->num_ri_running = 0;
> 	^^^^^^^^^^^^^^^^^^^^^^^^^
> 		I think this is not required.
> +	rp->nmissed = 0;
>
> +	rp->unregistering = 0;
> 	^^^^^^^^^^^^^^^^^^^^^^
> 		this can be also avoided.
> +
> +	/* Establish function entry probe point */
> +	if ((ret = register_kprobe(&rp->kp)) != 0) {
> +		kfree(rp->instances);
> +	}
> 	^^^^^^^^^^^^^^^^^^^^^
> 		you need to register first before adding instances to the 
> 		free_instances.
>  
The reason we did not register first before allocating/adding instances 
because we ant everything inplace when we let go the lock, otherwise 
that probe could get hit right after we release the lock and the 
instances are not setup.

> 	
> +	return ret;
> +}
> +
>
> +		
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^multiple handler will take care of all these right?
> 			 see my code below.
> +	if (rp->num_ri_running != 0) {
> +		int i;
> +		struct kretprobe_instance *ri;
> +		/* Make a copy of kretprobe so we can relinquish the 
> +		 * user's original.
> +		 */
> +		memcpy(rp_tmp, rp, sizeof(struct kretprobe));
> +		rp_tmp->unregistering = 1;
> +		for (i = 0 ; i < rp->maxactive; i++) {
> +			ri = rp->instances + i;
> +			ri->rp = rp_tmp;
> 	^^^^^^^^^^^^^^^^^^^^^^instead you use ri->ret_add to find you 
> 	if it is on afree list or on a kretprobe_inst_table[], by checking if NULL..
> +		}
> +}	
>  

We need the piece of code above in the case of (ie. a function has been 
recursive invoked and the return address has been replaced with the 
trampoline on stack)

> you can write unregister_kretprobe() something like this.
>
> void unregister_kretprobe(...........)
> {
>
>        unregister_kprobe(&rp->kp);
>        spin_lock_irqsave(&kprobe_lock, flags);
>
>        for (i = 0 ; i < rp->maxactive; i++) {
>                ri = rp->instances + i;
>                if (ri->ret_addr != NULL) 
> 		/* if not null means, it should be on kretprobe_inst_table[].*/
>                        hlist_del(&ri->hlist);
>        }
>        kfree(rp->instances);
>        spin_unlock_irqrestore(&kprobe_lock, flags);
> }
>