* [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
2012-10-28 17:39 [PATCH 0/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
@ 2012-10-28 17:39 ` Oleg Nesterov
2012-10-29 5:27 ` Ananth N Mavinakayanahalli
2012-10-28 17:39 ` [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers Oleg Nesterov
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2012-10-28 17:39 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior, Srikar Dronamraju
Cc: Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/powerpc/kernel/signal.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index a2dc757..3b99711 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
{
- if (thread_info_flags & _TIF_UPROBE) {
- clear_thread_flag(TIF_UPROBE);
+ if (thread_info_flags & _TIF_UPROBE)
uprobe_notify_resume(regs);
- }
if (thread_info_flags & _TIF_SIGPENDING)
do_signal(regs);
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
2012-10-28 17:39 ` [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume() Oleg Nesterov
@ 2012-10-29 5:27 ` Ananth N Mavinakayanahalli
2012-10-29 7:02 ` Srikar Dronamraju
2012-10-29 12:43 ` Oleg Nesterov
0 siblings, 2 replies; 14+ messages in thread
From: Ananth N Mavinakayanahalli @ 2012-10-29 5:27 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Sebastian Andrzej Siewior, Srikar Dronamraju, Anton Arapov,
Benjamin Herrenschmidt, Ingo Molnar, Peter Zijlstra,
Rabin Vincent, linux-kernel
On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
Hi Oleg,
> Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> arch/powerpc/kernel/signal.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index a2dc757..3b99711 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
>
> void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
But this _is_ do_notify_resume()... I don't see this flag cleared
anywhere else.
Did you have something else in mind?
Ananth
> {
> - if (thread_info_flags & _TIF_UPROBE) {
> - clear_thread_flag(TIF_UPROBE);
> + if (thread_info_flags & _TIF_UPROBE)
> uprobe_notify_resume(regs);
> - }
>
> if (thread_info_flags & _TIF_SIGPENDING)
> do_signal(regs);
> --
> 1.5.5.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
2012-10-29 5:27 ` Ananth N Mavinakayanahalli
@ 2012-10-29 7:02 ` Srikar Dronamraju
2012-10-29 12:43 ` Oleg Nesterov
1 sibling, 0 replies; 14+ messages in thread
From: Srikar Dronamraju @ 2012-10-29 7:02 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli
Cc: Oleg Nesterov, Sebastian Andrzej Siewior, Anton Arapov,
Benjamin Herrenschmidt, Ingo Molnar, Peter Zijlstra,
Rabin Vincent, linux-kernel
* Ananth N Mavinakayanahalli <ananth@in.ibm.com> [2012-10-29 10:57:07]:
> On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
>
> Hi Oleg,
>
> > Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> > arch/powerpc/kernel/signal.c | 4 +---
> > 1 files changed, 1 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > index a2dc757..3b99711 100644
> > --- a/arch/powerpc/kernel/signal.c
> > +++ b/arch/powerpc/kernel/signal.c
> > @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> >
> > void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
>
> But this _is_ do_notify_resume()... I don't see this flag cleared
> anywhere else.
>
> Did you have something else in mind?
uprobe_notify_resume() itself clears TIF_UPROBES.
This change is already part of -tip but not mainline.
Hence you might have missed it.
> > {
> > - if (thread_info_flags & _TIF_UPROBE) {
> > - clear_thread_flag(TIF_UPROBE);
> > + if (thread_info_flags & _TIF_UPROBE)
> > uprobe_notify_resume(regs);
> > - }
> >
> > if (thread_info_flags & _TIF_SIGPENDING)
> > do_signal(regs);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
2012-10-29 5:27 ` Ananth N Mavinakayanahalli
2012-10-29 7:02 ` Srikar Dronamraju
@ 2012-10-29 12:43 ` Oleg Nesterov
2012-10-29 13:28 ` Ananth N Mavinakayanahalli
2012-11-02 5:02 ` Srikar Dronamraju
1 sibling, 2 replies; 14+ messages in thread
From: Oleg Nesterov @ 2012-10-29 12:43 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli
Cc: Sebastian Andrzej Siewior, Srikar Dronamraju, Anton Arapov,
Benjamin Herrenschmidt, Ingo Molnar, Peter Zijlstra,
Rabin Vincent, linux-kernel
On 10/29, Ananth N Mavinakayanahalli wrote:
>
> On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
>
> Hi Oleg,
>
> > Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> > arch/powerpc/kernel/signal.c | 4 +---
> > 1 files changed, 1 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > index a2dc757..3b99711 100644
> > --- a/arch/powerpc/kernel/signal.c
> > +++ b/arch/powerpc/kernel/signal.c
> > @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> >
> > void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
>
> But this _is_ do_notify_resume()...
Argh. I'll fix the changelog, see v2 below.
> I don't see this flag cleared
> anywhere else.
>
> Did you have something else in mind?
Sorry for confusion.
As Srikar explained, it is cleared by uprobe_notify_resume(). See db023ea5
"uprobes: Move clear_thread_flag(TIF_UPROBE) to uprobe_notify_resume()", it
is already in Linus's tree.
Oleg.
------------------------------------------------------------------------------
Subject: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
Cleanup. No need to clear TIF_UPROBE, uprobe_notify_resume() does this.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/powerpc/kernel/signal.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index a2dc757..3b99711 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
{
- if (thread_info_flags & _TIF_UPROBE) {
- clear_thread_flag(TIF_UPROBE);
+ if (thread_info_flags & _TIF_UPROBE)
uprobe_notify_resume(regs);
- }
if (thread_info_flags & _TIF_SIGPENDING)
do_signal(regs);
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
2012-10-29 12:43 ` Oleg Nesterov
@ 2012-10-29 13:28 ` Ananth N Mavinakayanahalli
2012-11-02 5:02 ` Srikar Dronamraju
1 sibling, 0 replies; 14+ messages in thread
From: Ananth N Mavinakayanahalli @ 2012-10-29 13:28 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Sebastian Andrzej Siewior, Srikar Dronamraju, Anton Arapov,
Benjamin Herrenschmidt, Ingo Molnar, Peter Zijlstra,
Rabin Vincent, linux-kernel
On Mon, Oct 29, 2012 at 01:43:25PM +0100, Oleg Nesterov wrote:
> On 10/29, Ananth N Mavinakayanahalli wrote:
> >
> > On Sun, Oct 28, 2012 at 06:39:25PM +0100, Oleg Nesterov wrote:
> >
> > Hi Oleg,
> >
> > > Cleanup. No need to clear TIF_UPROBE, do_notify_resume() does this.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > > ---
> > > arch/powerpc/kernel/signal.c | 4 +---
> > > 1 files changed, 1 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > > index a2dc757..3b99711 100644
> > > --- a/arch/powerpc/kernel/signal.c
> > > +++ b/arch/powerpc/kernel/signal.c
> > > @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
> > >
> > > void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
> >
> > But this _is_ do_notify_resume()...
>
> Argh. I'll fix the changelog, see v2 below.
>
> > I don't see this flag cleared
> > anywhere else.
> >
> > Did you have something else in mind?
>
> Sorry for confusion.
>
> As Srikar explained, it is cleared by uprobe_notify_resume(). See db023ea5
> "uprobes: Move clear_thread_flag(TIF_UPROBE) to uprobe_notify_resume()", it
> is already in Linus's tree.
I was looking at rc2.. that explains it.
> Oleg.
>
> ------------------------------------------------------------------------------
> Subject: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
>
> Cleanup. No need to clear TIF_UPROBE, uprobe_notify_resume() does this.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Thanks Oleg!
Ananth
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
2012-10-29 12:43 ` Oleg Nesterov
2012-10-29 13:28 ` Ananth N Mavinakayanahalli
@ 2012-11-02 5:02 ` Srikar Dronamraju
1 sibling, 0 replies; 14+ messages in thread
From: Srikar Dronamraju @ 2012-11-02 5:02 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior,
Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
> Subject: [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume()
>
> Cleanup. No need to clear TIF_UPROBE, uprobe_notify_resume() does this.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/signal.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index a2dc757..3b99711 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -158,10 +158,8 @@ static int do_signal(struct pt_regs *regs)
>
> void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
> {
> - if (thread_info_flags & _TIF_UPROBE) {
> - clear_thread_flag(TIF_UPROBE);
> + if (thread_info_flags & _TIF_UPROBE)
> uprobe_notify_resume(regs);
> - }
>
> if (thread_info_flags & _TIF_SIGPENDING)
> do_signal(regs);
> --
> 1.5.5.1
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers
2012-10-28 17:39 [PATCH 0/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
2012-10-28 17:39 ` [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume() Oleg Nesterov
@ 2012-10-28 17:39 ` Oleg Nesterov
2012-10-29 5:33 ` Ananth N Mavinakayanahalli
2012-11-02 5:03 ` Srikar Dronamraju
2012-10-28 17:39 ` [PATCH 3/4] uprobes/x86: Cleanup the single-stepping code Oleg Nesterov
2012-10-28 17:39 ` [PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
3 siblings, 2 replies; 14+ messages in thread
From: Oleg Nesterov @ 2012-10-28 17:39 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior, Srikar Dronamraju
Cc: Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
No functional changes.
powerpc is the only user of arch_uprobe_enable/disable_step() helpers,
but they should die. They can not be used correctly, every arch needs
its own implementation (like x86 does). And they do not really help
even as initial-and-almost-working code, arch_uprobe_*_xol() hooks can
easily use user_enable/disable_single_step() directly.
Change arch_uprobe_*_step() to do nothing, and convert powerpc to use
ptrace helpers. This is equally wrong, powerpc needs the arch-specific
fixes.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/powerpc/kernel/uprobes.c | 6 ++++++
kernel/events/uprobes.c | 2 --
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index d2d46d1..bc77834 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -64,6 +64,8 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
autask->saved_trap_nr = current->thread.trap_nr;
current->thread.trap_nr = UPROBE_TRAP_NR;
regs->nip = current->utask->xol_vaddr;
+
+ user_enable_single_step(current);
return 0;
}
@@ -119,6 +121,8 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
* to be executed.
*/
regs->nip = utask->vaddr + MAX_UINSN_BYTES;
+
+ user_disable_single_step(current);
return 0;
}
@@ -162,6 +166,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
current->thread.trap_nr = utask->autask.saved_trap_nr;
instruction_pointer_set(regs, utask->vaddr);
+
+ user_disable_single_step(current);
}
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 672227a..916391e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1432,12 +1432,10 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
{
- user_enable_single_step(current);
}
void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
{
- user_disable_single_step(current);
}
/*
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers
2012-10-28 17:39 ` [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers Oleg Nesterov
@ 2012-10-29 5:33 ` Ananth N Mavinakayanahalli
2012-11-02 5:03 ` Srikar Dronamraju
1 sibling, 0 replies; 14+ messages in thread
From: Ananth N Mavinakayanahalli @ 2012-10-29 5:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Sebastian Andrzej Siewior, Srikar Dronamraju, Anton Arapov,
Benjamin Herrenschmidt, Ingo Molnar, Peter Zijlstra,
Rabin Vincent, linux-kernel
On Sun, Oct 28, 2012 at 06:39:28PM +0100, Oleg Nesterov wrote:
> No functional changes.
>
> powerpc is the only user of arch_uprobe_enable/disable_step() helpers,
> but they should die. They can not be used correctly, every arch needs
> its own implementation (like x86 does). And they do not really help
> even as initial-and-almost-working code, arch_uprobe_*_xol() hooks can
> easily use user_enable/disable_single_step() directly.
>
> Change arch_uprobe_*_step() to do nothing, and convert powerpc to use
> ptrace helpers. This is equally wrong, powerpc needs the arch-specific
> fixes.
Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
I will send a powerpc patch to directly use the MSR bits for stepping.
Ananth
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers
2012-10-28 17:39 ` [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers Oleg Nesterov
2012-10-29 5:33 ` Ananth N Mavinakayanahalli
@ 2012-11-02 5:03 ` Srikar Dronamraju
1 sibling, 0 replies; 14+ messages in thread
From: Srikar Dronamraju @ 2012-11-02 5:03 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior,
Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-10-28 18:39:28]:
> No functional changes.
>
> powerpc is the only user of arch_uprobe_enable/disable_step() helpers,
> but they should die. They can not be used correctly, every arch needs
> its own implementation (like x86 does). And they do not really help
> even as initial-and-almost-working code, arch_uprobe_*_xol() hooks can
> easily use user_enable/disable_single_step() directly.
>
> Change arch_uprobe_*_step() to do nothing, and convert powerpc to use
> ptrace helpers. This is equally wrong, powerpc needs the arch-specific
> fixes.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/uprobes.c | 6 ++++++
> kernel/events/uprobes.c | 2 --
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index d2d46d1..bc77834 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -64,6 +64,8 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> autask->saved_trap_nr = current->thread.trap_nr;
> current->thread.trap_nr = UPROBE_TRAP_NR;
> regs->nip = current->utask->xol_vaddr;
> +
> + user_enable_single_step(current);
> return 0;
> }
>
> @@ -119,6 +121,8 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> * to be executed.
> */
> regs->nip = utask->vaddr + MAX_UINSN_BYTES;
> +
> + user_disable_single_step(current);
> return 0;
> }
>
> @@ -162,6 +166,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>
> current->thread.trap_nr = utask->autask.saved_trap_nr;
> instruction_pointer_set(regs, utask->vaddr);
> +
> + user_disable_single_step(current);
> }
>
> /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 672227a..916391e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1432,12 +1432,10 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>
> void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
> {
> - user_enable_single_step(current);
> }
>
> void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
> {
> - user_disable_single_step(current);
> }
>
> /*
> --
> 1.5.5.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] uprobes/x86: Cleanup the single-stepping code
2012-10-28 17:39 [PATCH 0/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
2012-10-28 17:39 ` [PATCH 1/4] uprobes/powerpc: Don't clear TIF_UPROBE in do_notify_resume() Oleg Nesterov
2012-10-28 17:39 ` [PATCH 2/4] uprobes/powerpc: Do not use arch_uprobe_*_step() helpers Oleg Nesterov
@ 2012-10-28 17:39 ` Oleg Nesterov
2012-11-02 12:51 ` Srikar Dronamraju
2012-10-28 17:39 ` [PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
3 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2012-10-28 17:39 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior, Srikar Dronamraju
Cc: Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
No functional changes.
Now that default arch_uprobe_enable/disable_step() helpers do nothing,
x86 has no reason to reimplement them. Change arch_uprobe_*_xol() hooks
to do the necessary work and remove the x86-specific hooks.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/x86/kernel/uprobes.c | 54 +++++++++++++++-----------------------------
1 files changed, 19 insertions(+), 35 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index aafa555..c71025b 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -478,6 +478,11 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
regs->ip = current->utask->xol_vaddr;
pre_xol_rip_insn(auprobe, regs, autask);
+ autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
+ regs->flags |= X86_EFLAGS_TF;
+ if (test_tsk_thread_flag(current, TIF_BLOCKSTEP))
+ set_task_blockstep(current, false);
+
return 0;
}
@@ -603,6 +608,16 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
if (auprobe->fixups & UPROBE_FIX_CALL)
result = adjust_ret_addr(regs->sp, correction);
+ /*
+ * arch_uprobe_pre_xol() doesn't save the state of TIF_BLOCKSTEP
+ * so we can get an extra SIGTRAP if we do not clear TF. We need
+ * to examine the opcode to make it right.
+ */
+ if (utask->autask.saved_tf)
+ send_sig(SIGTRAP, current, 0);
+ else if (!(auprobe->fixups & UPROBE_FIX_SETF))
+ regs->flags &= ~X86_EFLAGS_TF;
+
return result;
}
@@ -647,6 +662,10 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
current->thread.trap_nr = utask->autask.saved_trap_nr;
handle_riprel_post_xol(auprobe, regs, NULL);
instruction_pointer_set(regs, utask->vaddr);
+
+ /* clear TF if it was set by us in arch_uprobe_pre_xol() */
+ if (!utask->autask.saved_tf)
+ regs->flags &= ~X86_EFLAGS_TF;
}
/*
@@ -676,38 +695,3 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
send_sig(SIGTRAP, current, 0);
return ret;
}
-
-void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
-{
- struct task_struct *task = current;
- struct arch_uprobe_task *autask = &task->utask->autask;
- struct pt_regs *regs = task_pt_regs(task);
-
- autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
-
- regs->flags |= X86_EFLAGS_TF;
- if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
- set_task_blockstep(task, false);
-}
-
-void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
-{
- struct task_struct *task = current;
- struct arch_uprobe_task *autask = &task->utask->autask;
- bool trapped = (task->utask->state == UTASK_SSTEP_TRAPPED);
- struct pt_regs *regs = task_pt_regs(task);
- /*
- * The state of TIF_BLOCKSTEP was not saved so we can get an extra
- * SIGTRAP if we do not clear TF. We need to examine the opcode to
- * make it right.
- */
- if (unlikely(trapped)) {
- if (!autask->saved_tf)
- regs->flags &= ~X86_EFLAGS_TF;
- } else {
- if (autask->saved_tf)
- send_sig(SIGTRAP, task, 0);
- else if (!(auprobe->fixups & UPROBE_FIX_SETF))
- regs->flags &= ~X86_EFLAGS_TF;
- }
-}
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] uprobes/x86: Cleanup the single-stepping code
2012-10-28 17:39 ` [PATCH 3/4] uprobes/x86: Cleanup the single-stepping code Oleg Nesterov
@ 2012-11-02 12:51 ` Srikar Dronamraju
0 siblings, 0 replies; 14+ messages in thread
From: Srikar Dronamraju @ 2012-11-02 12:51 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior,
Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-10-28 18:39:31]:
> No functional changes.
>
> Now that default arch_uprobe_enable/disable_step() helpers do nothing,
> x86 has no reason to reimplement them. Change arch_uprobe_*_xol() hooks
> to do the necessary work and remove the x86-specific hooks.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> arch/x86/kernel/uprobes.c | 54 +++++++++++++++-----------------------------
> 1 files changed, 19 insertions(+), 35 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index aafa555..c71025b 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -478,6 +478,11 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> regs->ip = current->utask->xol_vaddr;
> pre_xol_rip_insn(auprobe, regs, autask);
>
> + autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
> + regs->flags |= X86_EFLAGS_TF;
> + if (test_tsk_thread_flag(current, TIF_BLOCKSTEP))
> + set_task_blockstep(current, false);
> +
> return 0;
> }
>
> @@ -603,6 +608,16 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> if (auprobe->fixups & UPROBE_FIX_CALL)
> result = adjust_ret_addr(regs->sp, correction);
>
> + /*
> + * arch_uprobe_pre_xol() doesn't save the state of TIF_BLOCKSTEP
> + * so we can get an extra SIGTRAP if we do not clear TF. We need
> + * to examine the opcode to make it right.
> + */
> + if (utask->autask.saved_tf)
> + send_sig(SIGTRAP, current, 0);
> + else if (!(auprobe->fixups & UPROBE_FIX_SETF))
> + regs->flags &= ~X86_EFLAGS_TF;
> +
> return result;
> }
>
> @@ -647,6 +662,10 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> current->thread.trap_nr = utask->autask.saved_trap_nr;
> handle_riprel_post_xol(auprobe, regs, NULL);
> instruction_pointer_set(regs, utask->vaddr);
> +
> + /* clear TF if it was set by us in arch_uprobe_pre_xol() */
> + if (!utask->autask.saved_tf)
> + regs->flags &= ~X86_EFLAGS_TF;
> }
>
> /*
> @@ -676,38 +695,3 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> send_sig(SIGTRAP, current, 0);
> return ret;
> }
> -
> -void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
> -{
> - struct task_struct *task = current;
> - struct arch_uprobe_task *autask = &task->utask->autask;
> - struct pt_regs *regs = task_pt_regs(task);
> -
> - autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
> -
> - regs->flags |= X86_EFLAGS_TF;
> - if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
> - set_task_blockstep(task, false);
> -}
> -
> -void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
> -{
> - struct task_struct *task = current;
> - struct arch_uprobe_task *autask = &task->utask->autask;
> - bool trapped = (task->utask->state == UTASK_SSTEP_TRAPPED);
> - struct pt_regs *regs = task_pt_regs(task);
> - /*
> - * The state of TIF_BLOCKSTEP was not saved so we can get an extra
> - * SIGTRAP if we do not clear TF. We need to examine the opcode to
> - * make it right.
> - */
> - if (unlikely(trapped)) {
> - if (!autask->saved_tf)
> - regs->flags &= ~X86_EFLAGS_TF;
> - } else {
> - if (autask->saved_tf)
> - send_sig(SIGTRAP, task, 0);
> - else if (!(auprobe->fixups & UPROBE_FIX_SETF))
> - regs->flags &= ~X86_EFLAGS_TF;
> - }
> -}
> --
> 1.5.5.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks
2012-10-28 17:39 [PATCH 0/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
` (2 preceding siblings ...)
2012-10-28 17:39 ` [PATCH 3/4] uprobes/x86: Cleanup the single-stepping code Oleg Nesterov
@ 2012-10-28 17:39 ` Oleg Nesterov
2012-11-02 12:50 ` Srikar Dronamraju
3 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2012-10-28 17:39 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior, Srikar Dronamraju
Cc: Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
Kill arch_uprobe_enable/disable_step() hooks, they do nothing and
nobody needs them.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
include/linux/uprobes.h | 2 --
kernel/events/uprobes.c | 10 ----------
2 files changed, 0 insertions(+), 12 deletions(-)
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 2459457..2615c4d 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -101,8 +101,6 @@ extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
extern void uprobe_free_utask(struct task_struct *t);
extern void uprobe_copy_process(struct task_struct *t);
extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
-extern void __weak arch_uprobe_enable_step(struct arch_uprobe *arch);
-extern void __weak arch_uprobe_disable_step(struct arch_uprobe *arch);
extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
extern void uprobe_notify_resume(struct pt_regs *regs);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 916391e..02d7c5f 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1430,14 +1430,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
return uprobe;
}
-void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
-{
-}
-
-void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
-{
-}
-
/*
* Run handler and ask thread to singlestep.
* Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1491,7 +1483,6 @@ static void handle_swbp(struct pt_regs *regs)
goto out;
if (!pre_ssout(uprobe, regs, bp_vaddr)) {
- arch_uprobe_enable_step(&uprobe->arch);
utask->active_uprobe = uprobe;
utask->state = UTASK_SSTEP;
return;
@@ -1523,7 +1514,6 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
else
WARN_ON_ONCE(1);
- arch_uprobe_disable_step(&uprobe->arch);
put_uprobe(uprobe);
utask->active_uprobe = NULL;
utask->state = UTASK_RUNNING;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks
2012-10-28 17:39 ` [PATCH 4/4] uprobes: Kill arch_uprobe_enable/disable_step() hooks Oleg Nesterov
@ 2012-11-02 12:50 ` Srikar Dronamraju
0 siblings, 0 replies; 14+ messages in thread
From: Srikar Dronamraju @ 2012-11-02 12:50 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ananth N Mavinakayanahalli, Sebastian Andrzej Siewior,
Anton Arapov, Benjamin Herrenschmidt, Ingo Molnar,
Peter Zijlstra, Rabin Vincent, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-10-28 18:39:36]:
> Kill arch_uprobe_enable/disable_step() hooks, they do nothing and
> nobody needs them.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> include/linux/uprobes.h | 2 --
> kernel/events/uprobes.c | 10 ----------
> 2 files changed, 0 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 2459457..2615c4d 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -101,8 +101,6 @@ extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
> extern void uprobe_free_utask(struct task_struct *t);
> extern void uprobe_copy_process(struct task_struct *t);
> extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
> -extern void __weak arch_uprobe_enable_step(struct arch_uprobe *arch);
> -extern void __weak arch_uprobe_disable_step(struct arch_uprobe *arch);
> extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
> extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
> extern void uprobe_notify_resume(struct pt_regs *regs);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 916391e..02d7c5f 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1430,14 +1430,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> return uprobe;
> }
>
> -void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
> -{
> -}
> -
> -void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
> -{
> -}
> -
> /*
> * Run handler and ask thread to singlestep.
> * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> @@ -1491,7 +1483,6 @@ static void handle_swbp(struct pt_regs *regs)
> goto out;
>
> if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> - arch_uprobe_enable_step(&uprobe->arch);
> utask->active_uprobe = uprobe;
> utask->state = UTASK_SSTEP;
> return;
> @@ -1523,7 +1514,6 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
> else
> WARN_ON_ONCE(1);
>
> - arch_uprobe_disable_step(&uprobe->arch);
> put_uprobe(uprobe);
> utask->active_uprobe = NULL;
> utask->state = UTASK_RUNNING;
> --
> 1.5.5.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread