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

[Prasanna S Panchamukhi <prasanna at in dot ibm dot com>]

Hi,

Please find some comments on the code below.

Thanks
Prasanna

+
+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;
		
	
+	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?

+#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.
+
+
+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.
	
+	return ret;
+}
+
+void remove_kprobe(struct kprobe *kp, unsigned long flags)
+{
+	spin_unlock_irqrestore(&kprobe_lock, flags);
+	arch_remove_kprobe(kp);
+	spin_lock_irqsave(&kprobe_lock, flags);
+	*kp->addr = kp->opcode;
+	hlist_del(&kp->hlist);
+	flush_icache_range((unsigned long) kp->addr,
+			   (unsigned long) kp->addr + sizeof(kprobe_opcode_t));
+}
^^^^^^^^^^^^^^^^^^^^^^^this can be avoided, see my code below.
+
+void unregister_kretprobe(struct kretprobe *rp)
+{
+	unsigned long flags;
+	struct kretprobe *rp_tmp;
+	struct kprobe *old_p;
+
+	rp_tmp = kmalloc(sizeof(struct kretprobe), GFP_KERNEL);
+	BUG_ON(rp_tmp == NULL);
+	
+	spin_lock_irqsave(&kprobe_lock, flags);
+	old_p = get_kprobe(rp->kp.addr);
+	if (old_p && (old_p->pre_handler == aggr_pre_handler)) {
+		list_del(&rp->kp.list);
+		if (list_empty(&old_p->list)) {
+			remove_kprobe(old_p, flags);
+			kfree(old_p);
+		} 
+	} else if (old_p == &rp->kp) {
+		remove_kprobe(&rp->kp, flags);
+	}
+		
^^^^^^^^^^^^^^^^^^^^^^^^^^^^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..
+		}
+}	
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);
}

> I need to get the hold of the lock before unregistering, I could not the 
> current unregister_kprobe / unregistering_kprobe_single / 
> unregister_aggr_kprobe as is without any modification, hence this code 
> is here.
> 
> >Thanks
> >Prasanna
> >
> > 
> >
> 

-- 

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