diff mbox series

[1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()

Message ID 1043d06a0affed83a4a46dd29466e72820ee215d.1666262278.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive)
State Accepted
Commit 2fa9482334b0593b7edc371a13c0cca81daaa89e
Headers show
Series powerpc/kprobes: preempt related changes and cleanups | expand

Commit Message

Naveen N. Rao Oct. 20, 2022, 5:28 p.m. UTC
arch_prepare_kprobe() is called from register_kprobe() via
prepare_kprobe(), or through register_aggr_kprobe(), both with the
kprobe_mutex held. Per the comment for get_kprobe():
  /*
   * This routine is called either:
   *	- under the 'kprobe_mutex' - during kprobe_[un]register().
   *				OR
   *	- with preemption disabled - from architecture specific code.
   */

As such, there is no need to disable preemption around the call to
get_kprobe(). Drop the same.

Reported-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Nicholas Piggin Nov. 7, 2022, 10:14 a.m. UTC | #1
On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
> arch_prepare_kprobe() is called from register_kprobe() via
> prepare_kprobe(), or through register_aggr_kprobe(), both with the
> kprobe_mutex held. Per the comment for get_kprobe():
>   /*
>    * This routine is called either:
>    *	- under the 'kprobe_mutex' - during kprobe_[un]register().
>    *				OR
>    *	- with preemption disabled - from architecture specific code.
>    */

That comment should read [un]register_kprobe(), right?

>
> As such, there is no need to disable preemption around the call to
> get_kprobe(). Drop the same.

And prepare_kprobe() and register_aggr_kprobe() are both called with
kprobe_mutex held so you rely on the same protection. This seems to
fix a lost-resched bug with preempt kernels too.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index bd7b1a03545948..88f42de681e1f8 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -158,9 +158,7 @@ int arch_prepare_kprobe(struct kprobe *p)
>  		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
>  		ret = -EINVAL;
>  	}
> -	preempt_disable();
>  	prev = get_kprobe(p->addr - 1);
> -	preempt_enable_no_resched();
>  
>  	/*
>  	 * When prev is a ftrace-based kprobe, we don't have an insn, and it
> -- 
> 2.38.0
Naveen N. Rao Nov. 8, 2022, 10:33 a.m. UTC | #2
Nicholas Piggin wrote:
> On Fri Oct 21, 2022 at 3:28 AM AEST, Naveen N. Rao wrote:
>> arch_prepare_kprobe() is called from register_kprobe() via
>> prepare_kprobe(), or through register_aggr_kprobe(), both with the
>> kprobe_mutex held. Per the comment for get_kprobe():
>>   /*
>>    * This routine is called either:
>>    *	- under the 'kprobe_mutex' - during kprobe_[un]register().
>>    *				OR
>>    *	- with preemption disabled - from architecture specific code.
>>    */
> 
> That comment should read [un]register_kprobe(), right?

Ugh, yes!

> 
>>
>> As such, there is no need to disable preemption around the call to
>> get_kprobe(). Drop the same.
> 
> And prepare_kprobe() and register_aggr_kprobe() are both called with
> kprobe_mutex held so you rely on the same protection. This seems to
> fix a lost-resched bug with preempt kernels too.
> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Naveen
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index bd7b1a03545948..88f42de681e1f8 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -158,9 +158,7 @@  int arch_prepare_kprobe(struct kprobe *p)
 		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
 		ret = -EINVAL;
 	}
-	preempt_disable();
 	prev = get_kprobe(p->addr - 1);
-	preempt_enable_no_resched();
 
 	/*
 	 * When prev is a ftrace-based kprobe, we don't have an insn, and it