Re: [PATCH 2/5][djprobe] djprobe core patch

[Keshavamurthy at bambi dot jf dot intel dot com, Anil S <anil dot s dot keshavamurthy at intel dot com>]

Hi Masami,
	Sorry for the delayed response. My comments are inline.

On Thu, Oct 19, 2006 at 06:02:32PM +0900, Masami Hiramatsu wrote:
> I also updated API reference.
> 
> API Reference
> -------------
> The Djprobe API includes "register_djprobe" function,
> "unregister_djprobe" function and "commit_djprobes" function.
> Here are specifications for these functions and the
> associated probe handlers.

I am curious as to why you are introducing a new interface
for registering djprobes. Can't this be done by modifying the
existing register_kprobe()/unregister_kprobe() and under the
covers you can choose to implement djprobes. 

The benifit of hiding djprobes under kprobes would be 

1)Users of kernel probe need not have to maintain two
version of their instrumentation code (one with kprobes 
and one with djprobes).

2)If for some reason djprobes fails (might be  because there exists
a kprobes in that djprobe address + size range), then the user of the
probe interface has to try kprobes in order to succeed the registration.
However if you choose to implement djprobes under the covers of kprobes, then you can
transparently support the insertion of the probes, through aggregate kprobes instead
of failing the djprobes.

Also with this approach, today's systemtap can easily take advantage of djprobes,
since djprobes is implemented under the kprobes implementation. Since you are targeting
your code for mainline, don't worry about kabi of kprobes interface.

Oaky, some more comments.
1) I did not see where you check for whether the 
target instruction of the djprobe is relocatable.
Are you currently assuming that the target instruction is simply relocatable?

2) You are not fully populating the pt_regs which the probe handler receives,
( you have bogus values in eflags, eip, esp, etc) so I am not sure whether 
this is a must for instrumentation code who might need these values.

[...]	
> +int __kprobes djprobe_kprobe(struct kprobe *kp)
Did not see where you are using this function.
> +{
> +	return kp->pre_handler == __djprobe_pre_handler;
> +}
[...]

> +
> +static __always_inline
> +    struct djprobe_instance *__create_djprobe_instance(struct arch_djprobe_param
> +						       *param)
> +{
> +	struct djprobe_instance *djpi;
> +	void * addr = (void *)djprobe_param_address(param);
> +	/* allocate a new instance */
> +	djpi = kcalloc(1, sizeof(struct djprobe_instance), GFP_ATOMIC);
I think you can use GFP_KERNEL.

> +	if (djpi == NULL) {
> +		goto out;
> +	}
> +
> +	/* allocate stub */
> +	djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages);
> +	if (djpi->stub.insn == NULL) {
> +		__free_djprobe_instance(djpi);
kfree(djpi) should do.

> +		djpi = NULL;
> +		goto out;
> +	}
> +
> +	arch_prepare_djprobe_instance(djpi, param);

[...]
> +
> +	if ((ret = in_kprobes_functions((long)addr)) != 0)
> +		return ret;
> +
> +	mutex_lock(&djprobe_mutex);
> +
> +	/* check confliction with other djprobes */
> +	djpi = __get_djprobe_instance(addr, len);
> +	if (djpi) {
> +		if (djpi->kp.addr == addr) {
> +			djp->inst = djpi;	/* add to another instance */
> +			list_add_rcu(&djp->plist, &djpi->plist);
> +		} else {
> +			ret = -EEXIST;	/* other djprobes were inserted */
> +		}
> +		goto out;
> +	}
> +	/* check confliction with kprobes */
> +	for (i = 1; i < len; i++) {
> +		if (get_kprobe((void *)((long)addr + i))) {
> +			ret = -EEXIST;	/* a kprobes were inserted */
> +			goto out;
> +		}
> +	}
You need similar check in the kprobes registration, as we don;t want
to insert kprobes on the djprobe where you have jump target address.


regards,
Anil Keshavamurthy