RE: [discuss] [PATCH] utilization of kprobe_mutex is incorrect on x86_64

RE: [discuss] [PATCH] utilization of kprobe_mutex is incorrect on x86_64

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Andi Kleen [mailto:ak@suse.de]
>>Sent: 2005年9月29日 22:31
>>To: prasanna@in.ibm.com
>>Cc: Zhang, Yanmin; linux-kernel@vger.kernel.org; discuss@x86-64.org;
>>systemtap@sources.redhat.com; Keshavamurthy, Anil S
>>Subject: Re: [discuss] [PATCH] utilization of kprobe_mutex is incorrect on
>>x86_64
>>
>>On Thursday 29 September 2005 16:13, Prasanna S Panchamukhi wrote:
>>> >On Thu, Sep 29, 2005 at 08:43:44AM +0800, Zhang, Yanmin wrote:
>>> >>  <<kprobe_incorrect_kprobe_mutex_2.6.14-rc2_x86_64.patch>> I found it
>>> >> when reading the source codes. Basically, the bug could break
>>> >> kprobe_insn_pages under multi-thread environment. PPC arch also has the
>>> >> problem.
>>> >
>>> >Can you describe what the problem actually is?
>>>
>>> Andi,
>>>
>>> The up()/down() orders are incorrect in arch/x86_64/kprobes.c file while
>>> trying to get/remove a kprobes instruction slot in arch_prepare_kprobe()
>>> and arch_remove_kprobe() routines. Zhang's patch corrects this.
>>
>>What I meant is that someone should describe why they are incorrect.
>>I could probably figure it out from the code, but in general the standards
>>for changelogs are higher than just "bla is wrong". It should be more like
>>"bla doesn't do X, so bad thing Y happens, which causes crash Z". Please
>>follow this in future patches.

Andy,
Sorry for lack of detailed explanation. What Prasanna said is correct.
The up()/down() orders are incorrect in arch/x86_64/kprobes.c file. kprobe_mutext is used to protect the free kprobe instruction slot list. arch_prepare_kprobe applies for a slot from the free list, and arch_remove_kprobe returns a slot to the free list. The incorrect up()/down() orders to operate on kprobe_mutex fail to protect the free list. If 2 threads try to get/return kprobe instruction slot at the same time, the free slot list might be broken, or a free slot might be applied by 2 threads. My patch fixes it.

Re: [discuss] [PATCH] utilization of kprobe_mutex is incorrect on x86_64

[Prasanna S Panchamukhi]

>On Thu, Sep 29, 2005 at 08:43:44AM +0800, Zhang, Yanmin wrote:
>>  <<kprobe_incorrect_kprobe_mutex_2.6.14-rc2_x86_64.patch>> I found it
>> when reading the source codes. Basically, the bug could break
>> kprobe_insn_pages under multi-thread environment. PPC arch also has the
>> problem.

>Can you describe what the problem actually is? 

Andi,

The up()/down() orders are incorrect in arch/x86_64/kprobes.c file while
trying to get/remove a kprobes instruction slot in arch_prepare_kprobe() 
and arch_remove_kprobe() routines. Zhang's patch corrects this.


Thanks
Prasanna
-- 

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

Re: [discuss] [PATCH] utilization of kprobe_mutex is incorrect on x86_64

[Andi Kleen]

On Thursday 29 September 2005 16:13, Prasanna S Panchamukhi wrote:
> >On Thu, Sep 29, 2005 at 08:43:44AM +0800, Zhang, Yanmin wrote:
> >>  <<kprobe_incorrect_kprobe_mutex_2.6.14-rc2_x86_64.patch>> I found it
> >> when reading the source codes. Basically, the bug could break
> >> kprobe_insn_pages under multi-thread environment. PPC arch also has the
> >> problem.
> >
> >Can you describe what the problem actually is?
>
> Andi,
>
> The up()/down() orders are incorrect in arch/x86_64/kprobes.c file while
> trying to get/remove a kprobes instruction slot in arch_prepare_kprobe()
> and arch_remove_kprobe() routines. Zhang's patch corrects this.

What I meant is that someone should describe why they are incorrect.
I could probably figure it out from the code, but in general the standards
for changelogs are higher than just "bla is wrong". It should be more like
"bla doesn't do X, so bad thing Y happens, which causes crash Z". Please 
follow this in future patches.

-Andi

[PATCH] utilization of kprobe_mutex is incorrect on x86_64

[Zhang, Yanmin]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

 <<kprobe_incorrect_kprobe_mutex_2.6.14-rc2_x86_64.patch>> I found it
when reading the source codes. Basically, the bug could break
kprobe_insn_pages under multi-thread environment. PPC arch also has the
problem.
Here is the patch against x86_64.

Signed-off-by: Zhang Yanmin <Yanmin.zhang@intel.com>



[-- Attachment #2: kprobe_incorrect_kprobe_mutex_2.6.14-rc2_x86_64.patch --]
[-- Type: application/octet-stream, Size: 943 bytes --]

diff -Nraup linux-2.6.14-rc2/arch/x86_64/kernel/kprobes.c linux-2.6.14-rc2_fix/arch/x86_64/kernel/kprobes.c
--- linux-2.6.14-rc2/arch/x86_64/kernel/kprobes.c	2005-09-29 08:35:46.000000000 +0800
+++ linux-2.6.14-rc2_fix/arch/x86_64/kernel/kprobes.c	2005-09-29 08:36:27.000000000 +0800
@@ -77,9 +77,9 @@ static inline int is_IF_modifier(kprobe_
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	/* insn: must be on special executable page on x86_64. */
-	up(&kprobe_mutex);
-	p->ainsn.insn = get_insn_slot();
 	down(&kprobe_mutex);
+	p->ainsn.insn = get_insn_slot();
+	up(&kprobe_mutex);
 	if (!p->ainsn.insn) {
 		return -ENOMEM;
 	}
@@ -231,9 +231,9 @@ void __kprobes arch_disarm_kprobe(struct
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	up(&kprobe_mutex);
-	free_insn_slot(p->ainsn.insn);
 	down(&kprobe_mutex);
+	free_insn_slot(p->ainsn.insn);
+	up(&kprobe_mutex);
 }
 
 static inline void save_previous_kprobe(void)

Re: [discuss] [PATCH] utilization of kprobe_mutex is incorrect on x86_64

[Andi Kleen]

On Thu, Sep 29, 2005 at 08:43:44AM +0800, Zhang, Yanmin wrote:
>  <<kprobe_incorrect_kprobe_mutex_2.6.14-rc2_x86_64.patch>> I found it
> when reading the source codes. Basically, the bug could break
> kprobe_insn_pages under multi-thread environment. PPC arch also has the
> problem.

Can you describe what the problem actually is? 

-Andi

> Here is the patch against x86_64.
> 
> Signed-off-by: Zhang Yanmin <Yanmin.zhang@intel.com>
> 
>