Re: [PATCH] stop_cpu() needs to use raw_smp_processor_id()

[Maran <maranp@linux.vnet.ibm.com>]

Heiko Carstens wrote:
> On Wed, Jun 17, 2009 at 12:31:56PM +0530, Maran wrote:
>   
>> Heiko Carstens wrote:
>>     
>>> On Tue, Jun 16, 2009 at 04:49:41PM +0530, Ananth N Mavinakayanahalli wrote:
>>>   
>>>       
>>>> With DEBUG_PREEMPT on, on an s390 box, we are seeing:
>>>>
>>>> BUG: using smp_processor_id() in preemptible [00000000] code: 
>>>> insmod/3006  caller is arch_arm_kprobe+0x2c/0x90                      
>>>>                   CPU: 0 Not tainted 
>>>> 2.6.30-rc8-tipjun10-02053-g39ae214 #7             Process insmod 
>>>> (pid: 3006, task: 000000007df31410, ksp: 000000007e087a28)   
>>>> 	0000000000000000 000000007e087c38 0000000000000002 0000000000000000
>>>> 	000000007e087cd8 000000007e087c50 000000007e087c50 0000000000048156  
>>>> 	00000000003e4de8 000000000002b830 0000000000000000 0000000000000000  
>>>> 	0000000000000000 000000007e087c38 ffffffff0000000d 000000007e087ca8  
>>>> 	000000000040a780 000000000001635c 000000007e087c38 000000007e087c80  
>>>> Call Trace:                                                           
>>>>      ([<00000000000162e6>] show_trace+0x13a/0x148)                    
>>>>            [<00000000001f123a>] debug_smp_processor_id+0x136/0x13c    
>>>>                 [<00000000003ddbd8>] arch_arm_kprobe+0x2c/0x90        
>>>>                      [<00000000003df8cc>] register_kprobe+0x630/0x684 
>>>>                           [<000003e000241056>] kprobe_init+0x56/0xa8 
>>>> [kprobe_example]                [<00000000000120c2>] 
>>>> do_one_initcall+0x3e/0x16c                            
>>>> [<000000000007cb4c>] SyS_init_module+0xc4/0x1fc                       
>>>>      [<0000000000027f42>] sysc_noemu+0x10/0x16                        
>>>>           [<0000004a4493324e>] 0x4a4493324e           
>>>>
>>>>         
>> With the current linus tree (git 10), kprobe breaks. Even smoke tests  
>> are failed in s390x box.
>> With 2.6.30, the same warning is raised even with CONFIG_SMP enabled. I  
>> have pasted the config file used.
>>     
>
> How about the patch below? Works for me. modprobe kprobe_example and the
> kprobes selftest work for me.
>
> Subject: [PATCH] kprobes: defer setting of ctlblk state
>
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> get_krobe_ctlblk returns a per cpu kprobe control block which holds
> the state of the current cpu wrt to kprobe.
> When inserting/removing a kprobe the state of the cpu which replaces
> the code is changed to KPROBE_SWAP_INST. This however is done when
> preemption is still enabled. So the state of the current cpu doesn't
> necessarily reflect the real state.
> To fix this move the code that changes the state to non-preemptible
> context.
>
> Reported-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  arch/s390/kernel/kprobes.c |   18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> Index: linux-2.6/arch/s390/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.orig/arch/s390/kernel/kprobes.c
> +++ linux-2.6/arch/s390/kernel/kprobes.c
> @@ -154,39 +154,35 @@ void __kprobes get_instruction_type(stru
>
>  static int __kprobes swap_instruction(void *aref)
>  {
> +	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	unsigned long status = kcb->kprobe_status;
>  	struct ins_replace_args *args = aref;
> +	int rc;
>
> -	return probe_kernel_write(args->ptr, &args->new, sizeof(args->new));
> +	kcb->kprobe_status = KPROBE_SWAP_INST;
> +	rc = probe_kernel_write(args->ptr, &args->new, sizeof(args->new));
> +	kcb->kprobe_status = status;
> +	return rc;
>  }
>
>  void __kprobes arch_arm_kprobe(struct kprobe *p)
>  {
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> -	unsigned long status = kcb->kprobe_status;
>  	struct ins_replace_args args;
>
>  	args.ptr = p->addr;
>  	args.old = p->opcode;
>  	args.new = BREAKPOINT_INSTRUCTION;
> -
> -	kcb->kprobe_status = KPROBE_SWAP_INST;
>  	stop_machine(swap_instruction, &args, NULL);
> -	kcb->kprobe_status = status;
>  }
>
>  void __kprobes arch_disarm_kprobe(struct kprobe *p)
>  {
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> -	unsigned long status = kcb->kprobe_status;
>  	struct ins_replace_args args;
>
>  	args.ptr = p->addr;
>  	args.old = BREAKPOINT_INSTRUCTION;
>  	args.new = p->opcode;
> -
> -	kcb->kprobe_status = KPROBE_SWAP_INST;
>  	stop_machine(swap_instruction, &args, NULL);
> -	kcb->kprobe_status = status;
>  }
>
>  void __kprobes arch_remove_kprobe(struct kprobe *p)
>   
Hi Heiko,
modprobe kprobe_example and kprobe sanity test passed on applying the 
patch. Thanks!