Re: [PATCH 1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()
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 Thanks, Naveen
Re: [PATCH 1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()
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
>
> Reported-by: Nicholas Piggin
> Signed-off-by: Naveen N. Rao
> ---
> 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
[PATCH 1/5] powerpc/kprobes: Remove preempt disable around call to get_kprobe() in arch_prepare_kprobe()
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
Signed-off-by: Naveen N. Rao
---
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
