From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25438 invoked by alias); 14 Nov 2005 02:03:43 -0000 Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org Received: (qmail 25427 invoked by uid 22791); 14 Nov 2005 02:03:40 -0000 X-MimeOLE: Produced By Microsoft Exchange V6.5.7226.0 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: quoted-printable Subject: RE: [Patch 2/3][Djprobe] Djprobe update for linux-2.6.14-mm1 Date: Mon, 14 Nov 2005 02:03:00 -0000 Message-ID: <8126E4F969BA254AB43EA03C59F44E8403DB7B23@pdsmsx404> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [Patch 2/3][Djprobe] Djprobe update for linux-2.6.14-mm1 Thread-Index: AcXm9PGHH0PH2D3qS8myW0as7ESvJwBx4Eag From: "Zhang, Yanmin" To: "Masami Hiramatsu" Cc: , "Satoshi Oshima" , "Yumiko Sugita" , "Hideo Aoki" , "Keshavamurthy, Anil S" X-OriginalArrivalTime: 14 Nov 2005 02:03:22.0012 (UTC) FILETIME=[9706A5C0:01C5E8BF] X-Scanned-By: MIMEDefang 2.52 on 10.7.209.16 X-SW-Source: 2005-q4/txt/msg00197.txt.bz2 >>-----Original Message----- >>From: Masami Hiramatsu [mailto:hiramatu@sdl.hitachi.co.jp] >>Sent: 2005=C4=EA11=D4=C212=C8=D5 3:20 >>To: Zhang, Yanmin >>Cc: systemtap@sources.redhat.com; Satoshi Oshima; Yumiko Sugita; Hideo Ao= ki; >>Keshavamurthy, Anil S >>Subject: Re: [Patch 2/3][Djprobe] Djprobe update for linux-2.6.14-mm1 >> >>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 =3D kcalloc(1, sizeof(struct djprobe_instance), GFP_ATOMIC); >>>>>+ if (djpi =3D=3D NULL) { >>>>>+ goto out; >>>>>+ } >>>>>+ /* allocate stub */ >>>>>+ djpi->stub.insn =3D __get_insn_slot(&djprobe_insn_pages); >>>>>+ if (djpi->stub.insn =3D=3D NULL) { >>> >>> [YM] If coming here, djpi->plist is not initiated. >>> So __free_djprobe_instance=3D>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 s= ize) >>>>>+{ >>>>>+ struct djprobe_instance *djpi; >>>>>+ struct kprobe *kp; >>>>>+ int ret =3D 0, i; >>>>>+ >>>>>+ BUG_ON(in_interrupt()); >>>>>+ >>>>>+ if (size > ARCH_STUB_INSN_MAX || size < ARCH_STUB_INSN_MIN) >>>>>+ return -EINVAL; >>>>>+ >>>>>+ if ((ret =3D in_kprobes_functions((unsigned long)addr)) !=3D 0) >>>>>+ return ret; >>>>>+ >>>>>+ down(&djprobe_mutex); >>>>>+ INIT_LIST_HEAD(&djp->plist); >>>>>+ /* check confliction with other djprobes */ >>>>>+ djpi =3D __get_djprobe_instance(addr, size); >>>>>+ if (djpi) { >>>>>+ if (djpi->kp.addr =3D=3D addr) { >>>>>+ djp->inst =3D djpi; /* add to another instance */ >>>>>+ list_add_rcu(&djp->plist, &djpi->plist); >>>>>+ } else { >>>>>+ ret =3D -EEXIST; /* other djprobes were inserted */ >>>>>+ } >>>>>+ goto out; >>>>>+ } >>>>>+ djpi =3D __create_djprobe_instance(djp, addr, size); >>>>>+ if (djpi =3D=3D NULL) { >>>>>+ ret =3D -ENOMEM; >>>>>+ goto out; >>>>>+ } >>>>>+ >>>>>+ /* check confliction with kprobes */ >>>>>+ for (i =3D 0; i < size; i++) { >>>>>+ kp =3D 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 =3D register_kprobe(&djpi->kp); >> if (ret < 0) { >> fail: >> djpi->kp.addr =3D NULL; >> djp->inst =3D NULL; >> list_del_rcu(&djp->plist); >> __free_djprobe_instance(djpi); >> } else { >> /* next, check confliction with kprobes */ >> for (i =3D 0; i < size; i++) { >> kp =3D get_kprobe((void *)((long)addr + i)); >> if (kp !=3D NULL && kp !=3D &djpi->kp) { >> ret =3D -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 =3D 0; >> unsigned long flags =3D 0; >> struct kprobe *old_p; >> >> if ((ret =3D in_kprobes_functions((unsigned long) p->addr)) !=3D = 0) >> return ret; >>#ifdef CONFIG_DJPROBE >> down(&djprobe_mutex); >> if (p->pre_handler !=3D djprobe_pre_handler && >> get_djprobe_instance(p->addr, 1) !=3D NULL) >> return -EEXIST; >>#endif /* CONFIG_DJPROBE */ >> if ((ret =3D arch_prepare_kprobe(p)) !=3D 0) >> goto rm_kprobe; >> >> p->nmissed =3D 0; >> spin_lock_irqsave(&kprobe_lock, flags); >> old_p =3D get_kprobe(p->addr); >> if (old_p) { >> ret =3D 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 =3D=3D -EEXIST) >> arch_remove_kprobe(p); >> return ret; >>} [YM] It's reasonable. In function register_kprobe,=20 1) get_djprobe_instance should be __get_djprobe_instance if djprobe_mutex i= s used. 2) Release djprobe_mutex before " return -EEXIST". 3) Parameter size of call to get_djprobe_instance is always 1 here. How abo= ut to change it to ARCH_STUB_INSN_MAX? One more comment on your 3rd patch, how about to change: +#define ARCH_STUB_SIZE ((long)&arch_tmpl_stub_end - (long)&arch_tmpl_stub_= entry) to +#define ARCH_STUB_SIZE (((long)&arch_tmpl_stub_end - (long)&arch_tmpl_stub= _entry)/sizeof(kprobe_opcode_t)) On ia32, sizeof(kprobe_opcode_t) is equal to 1, but on other platform, it m= ight not be. Just to make it clearer.