Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes

[Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>]

On Thu, Sep 29, 2005 at 09:59:31PM +0900, Masami Hiramatsu wrote:

Few comments on first glance...

[...]

> +int __kprobes register_djprobe(struct djprobe * djp)
> +{
> +	struct djprobe_instance *djpi;
> +	struct kprobe *kp;
> +	int ret = 0, i;
> +	
> +	if (djp == NULL || djp->addr == NULL ||
> +	    djp->size > ARCH_STUB_INSN_MAX ||
> +	    djp->size < ARCH_STUB_INSN_MIN ||
> +	    djp->inst != NULL)
> +		return -EINVAL;
> +
> +	if ((ret = in_kprobes_functions((unsigned long) djp->addr)) != 0)
> +		return ret;
> +
> +	spin_lock(&djprobe_lock);

Please use _irqsave/_irqrestore versions at all places.

> +	/* check confliction with other djprobes */
> +	djpi = __get_djprobe_instance(djp->addr, djp->size);
> +	if (djpi) {
> +		if (djpi->kp.addr == djp->addr && DJPI_EMPTY(djpi)) {
> +			djp->inst = djpi;
> +			djpi->djp = djp; /*TODO: use list*/
> +			goto out;
> +		} else {
> +			ret = -EEXIST; /* a djprobe were inserted */
> +			goto out;
> +		}
> +	}
> +	/* check confliction with kprobes */
> +	for ( i=0; i < djp->size; i++) {
> +		kp = get_kprobe((void*)((long)djp->addr+i));
> +		if (kp != NULL) {
> +			ret = -EEXIST; /* a kprobes were inserted */
> +			goto out;
> +		}
> +	}
> +	/* make a new instance */
> +	djpi = kmalloc(sizeof(struct djprobe_instance),GFP_KERNEL);

You are under a spinlock... this kmalloc may sleep.

> +	if (djpi == NULL) {
> +		ret = -ENOMEM; /* memory allocation error */
> +		goto out;
> +	}
> +	memset(djpi, 0, sizeof(struct djprobe_instance)); /* for kprobe */
> +	/* attach */
> +	djp->inst = djpi;
> +	djpi->djp = djp; /*TODO: use list*/
> +	djpi->kp.addr = djp->addr;
> +	INIT_LIST_HEAD(&djpi->list);
> +	list_add(&djpi->list, &djprobe_list);
> +
> +	/* prepare stub */
> +	djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages);
> +	if (djpi->stub.insn == NULL) {
> +		kfree(djpi);
> +		ret = -ENOMEM; /* memory allocation error */
> +		goto out;
> +	}
> +	djpi->kp.pre_handler = djprobe_bypass_handler;
> +	arch_prepare_djprobe_instance(djpi, djp->size); /*TODO : remove size*/
> +
> +	ret = install_djprobe_instance(djpi);
> +	if (ret < 0) { /* failed to install */
> +		djp->inst = NULL;
> +		djpi->kp.addr = NULL;
> +		__free_djprobe_instance(djpi);
> +	}
> +out:
> +	spin_unlock(&djprobe_lock);
> +	return ret;
> +}
> +
> +void __kprobes unregister_djprobe(struct djprobe * djp)
> +{
> +	struct djprobe_instance *djpi;
> +	if (djp == NULL || djp->inst == NULL)
> +		return ;
> +
> +	djpi = djp->inst;
> +	spin_lock(&djprobe_lock);
> +	djp->inst = NULL;
> +	djpi->djp = NULL; /*TODO: use list*/
> +	if (DJPI_EMPTY(djpi)) {
> +		uninstall_djprobe_instance(djpi);
> +	}
> +	spin_unlock(&djprobe_lock);
> +}
> +
> +#else /* ARCH_SUPPORTS_DJPROBES */
> +int __kprobes register_djprobe(struct djprobe *p)
> +{
> +	if (p!=NULL) {

Follow CodingStyle please! There are a few other places in the existing
kprobes code that also need a CodingStyle cleanup, but that is for a
later patch.

Ananth