Re: [Patch 2/3][Djprobe] Djprobe update for linux-2.6.14-mm1

[Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>]

Hi,

Thank you for your review!

Zhang, Yanmin wrote:
>>>-----Original Message-----
>>>From: systemtap-owner@sourceware.org [mailto:systemtap-owner@sourceware.org]
>>>On Behalf Of Masami Hiramatsu
>>>Sent: 2005/11/8 21:26
>>>To: systemtap@sources.redhat.com
>>>Cc: Satoshi Oshima; Yumiko Sugita; Hideo Aoki
>>>Subject: [Patch 2/3][Djprobe] Djprobe update for linux-2.6.14-mm1
>>>
>>>Hi,
>>>
>>>This patch is the architecture independant part of djprobe.
>>>+static inline
>>>+    struct djprobe_instance *__create_djprobe_instance(struct djprobe *djp,
>>>+						       void *addr, int size)
>>>+{
>>>+	struct djprobe_instance *djpi;
>>>+	/* allocate a new instance */
>>>+	djpi = kcalloc(1, sizeof(struct djprobe_instance), GFP_ATOMIC);
>>>+	if (djpi == NULL) {
>>>+		goto out;
>>>+	}
>>>+	/* allocate stub */
>>>+	djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages);
>>>+	if (djpi->stub.insn == NULL) {
>
> [YM] If coming here, djpi->plist is not initiated.
> So __free_djprobe_instance=>hlist_del will cause panic.
> How about to move the INIT_LIST_HEAD(&djpi->plist) just after kcalloc?

Thanks for finding that. I will fix it so.

>>>+int __kprobes register_djprobe(struct djprobe *djp, void *addr, int size)
>>>+{
>>>+	struct djprobe_instance *djpi;
>>>+	struct kprobe *kp;
>>>+	int ret = 0, i;
>>>+
>>>+	BUG_ON(in_interrupt());
>>>+
>>>+	if (size > ARCH_STUB_INSN_MAX || size < ARCH_STUB_INSN_MIN)
>>>+		return -EINVAL;
>>>+
>>>+	if ((ret = in_kprobes_functions((unsigned long)addr)) != 0)
>>>+		return ret;
>>>+
>>>+	down(&djprobe_mutex);
>>>+	INIT_LIST_HEAD(&djp->plist);
>>>+	/* check confliction with other djprobes */
>>>+	djpi = __get_djprobe_instance(addr, size);
>>>+	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;
>>>+	}
>>>+	djpi = __create_djprobe_instance(djp, addr, size);
>>>+	if (djpi == NULL) {
>>>+		ret = -ENOMEM;
>>>+		goto out;
>>>+	}
>>>+
>>>+	/* check confliction with kprobes */
>>>+	for (i = 0; i < size; i++) {
>>>+		kp = get_kprobe((void *)((long)addr + i));
>
> [YM] There is a race between get_kprobe and register_kprobe without
> locking kprobe_lock. Could register_kprobe to check if the address is
> in a JTPR of registered djprobe? I think djprobe and kprobe could
> share the same spin_lock, namely kprobe_lock.

hmm, but __check_safety() may sleep. So spin-lock will cause dead-lock.
I think it can avoid race condition by following two changes.

1) delay checking confliction like below.

       /* first, register as a kprobe.
	if there is another competitor, this waits until it registered */
        ret = register_kprobe(&djpi->kp);
        if (ret < 0) {
       fail:
                djpi->kp.addr = NULL;
                djp->inst = NULL;
                list_del_rcu(&djp->plist);
                __free_djprobe_instance(djpi);
        } else {
                /* next, check confliction with kprobes */
                for (i = 0; i < size; i++) {
                        kp = get_kprobe((void *)((long)addr + i));
                        if (kp != NULL && kp != &djpi->kp) {
                                ret = -EEXIST;  /* other kprobes were inserted */
                                goto fail;
                        }
                }
                __check_safety();
                arch_install_djprobe_instance(djpi);
        }


2) share the mutex of djprobe with kprobes like below.

int __kprobes register_kprobe(struct kprobe *p)
{
        int ret = 0;
        unsigned long flags = 0;
        struct kprobe *old_p;

        if ((ret = in_kprobes_functions((unsigned long) p->addr)) != 0)
                return ret;
#ifdef CONFIG_DJPROBE
        down(&djprobe_mutex);
        if (p->pre_handler != djprobe_pre_handler &&
            get_djprobe_instance(p->addr, 1) != NULL)
                return -EEXIST;
#endif /* CONFIG_DJPROBE */
        if ((ret = arch_prepare_kprobe(p)) != 0)
                goto rm_kprobe;

        p->nmissed = 0;
        spin_lock_irqsave(&kprobe_lock, flags);
        old_p = get_kprobe(p->addr);
        if (old_p) {
                ret = register_aggr_kprobe(old_p, p);
                goto out;
        }

        arch_copy_kprobe(p);
        INIT_HLIST_NODE(&p->hlist);
        hlist_add_head_rcu(&p->hlist,
                       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);

        arch_arm_kprobe(p);

out:
        spin_unlock_irqrestore(&kprobe_lock, flags);
rm_kprobe:
#ifdef CONFIG_DJPROBE
        up(&djprobe_mutex);
#endif /* CONFIG_DJPROBE */
        if (ret == -EEXIST)
                arch_remove_kprobe(p);
        return ret;
}

What would you think about this?

Best Regards,

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp