I was trying to get the address of instruction to be executed
next after the kprobed instruction. But regs->eip in post_handler()
contains value which is useless to the user. It's pre-corrected value.
This value is difficult to use without access to resume_execution(), which
is not exported anyway.
I moved the invocation of post_handler() to *after* resume_execution().
Now regs->eip contains meaningful value in post_handler().
I do not think this change breaks any backward-compatibility.
To make meaning of the old value, post_handler() would need access to
resume_execution() which is not exported. I have difficulty to believe
that previous, uncorrected, regs->eip can be meaningfully used in
post_handler().
Signed-off-by: Yakov Lerner <[email protected]>
---
arch/x86/kernel/kprobes.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index 34a5912..60392e2 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -858,15 +858,15 @@ static int __kprobes post_kprobe_handler(struct pt_regs *regs)
if (!cur)
return 0;
+ resume_execution(cur, regs, kcb);
+ regs->flags |= kcb->kprobe_saved_flags;
+ trace_hardirqs_fixup_flags(regs->flags);
+
if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
kcb->kprobe_status = KPROBE_HIT_SSDONE;
cur->post_handler(cur, regs, 0);
}
- resume_execution(cur, regs, kcb);
- regs->flags |= kcb->kprobe_saved_flags;
- trace_hardirqs_fixup_flags(regs->flags);
-
/* Restore back the original saved kprobes variables and continue. */
if (kcb->kprobe_status == KPROBE_REENTER) {
restore_previous_kprobe(kcb);
--
1.5.4.4
On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
>
> I was trying to get the address of instruction to be executed
> next after the kprobed instruction. But regs->eip in post_handler()
> contains value which is useless to the user. It's pre-corrected value.
> This value is difficult to use without access to resume_execution(), which
> is not exported anyway.
> I moved the invocation of post_handler() to *after* resume_execution().
> Now regs->eip contains meaningful value in post_handler().
>
> I do not think this change breaks any backward-compatibility.
> To make meaning of the old value, post_handler() would need access to
> resume_execution() which is not exported. I have difficulty to believe
> that previous, uncorrected, regs->eip can be meaningfully used in
> post_handler().
resume_execution() exists not just for the program counter fixups after
out-of-line singlestepping, but is also as an insurance to put the
program counter back to the correct address in case the user's
post_handler() mucks around with it. That isn't possible with this
change :-(
Ananth
On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
<[email protected]> wrote:
> On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
> >
> > I was trying to get the address of instruction to be executed
> > next after the kprobed instruction. But regs->eip in post_handler()
> > contains value which is useless to the user. It's pre-corrected value.
> > This value is difficult to use without access to resume_execution(), which
> > is not exported anyway.
> > I moved the invocation of post_handler() to *after* resume_execution().
> > Now regs->eip contains meaningful value in post_handler().
> >
> > I do not think this change breaks any backward-compatibility.
> > To make meaning of the old value, post_handler() would need access to
> > resume_execution() which is not exported. I have difficulty to believe
> > that previous, uncorrected, regs->eip can be meaningfully used in
> > post_handler().
>
> resume_execution() exists not just for the program counter fixups after
> out-of-line singlestepping, but is also as an insurance to put the
> program counter back to the correct address in case the user's
> post_handler() mucks around with it. That isn't possible with this
> change :-(
I see your point. This can be prevented by saving and restoring regs->ip
around the post_handler() call, no ? Current code is beautiful. Saving and
restoring regs->ip would make this place look ugly.
Otoh, if the post_handler() wants to crash the kernel, it can do it
in thousand ways, not just by trashing regs->ip, no ?
Yakov
On Mon, Mar 17, 2008 at 12:59:05PM +0200, Yakov Lerner wrote:
> On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
> <[email protected]> wrote:
> > On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
> > >
> > > I was trying to get the address of instruction to be executed
> > > next after the kprobed instruction. But regs->eip in post_handler()
> > > contains value which is useless to the user. It's pre-corrected value.
> > > This value is difficult to use without access to resume_execution(), which
> > > is not exported anyway.
> > > I moved the invocation of post_handler() to *after* resume_execution().
> > > Now regs->eip contains meaningful value in post_handler().
> > >
> > > I do not think this change breaks any backward-compatibility.
> > > To make meaning of the old value, post_handler() would need access to
> > > resume_execution() which is not exported. I have difficulty to believe
> > > that previous, uncorrected, regs->eip can be meaningfully used in
> > > post_handler().
> >
> > resume_execution() exists not just for the program counter fixups after
> > out-of-line singlestepping, but is also as an insurance to put the
> > program counter back to the correct address in case the user's
> > post_handler() mucks around with it. That isn't possible with this
> > change :-(
>
> I see your point. This can be prevented by saving and restoring regs->ip
> around the post_handler() call, no ? Current code is beautiful. Saving and
> restoring regs->ip would make this place look ugly.
>
> Otoh, if the post_handler() wants to crash the kernel, it can do it
> in thousand ways, not just by trashing regs->ip, no ?
Of course, there still are other ways to shoot yourself in the foot with
the post_handler(), but, atleast for cases we can control, we need to do
the right thing.
Ananth
Ananth N Mavinakayanahalli wrote:
> On Mon, Mar 17, 2008 at 12:59:05PM +0200, Yakov Lerner wrote:
>> On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
>> <[email protected]> wrote:
>>> On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
>>> >
>>> > I was trying to get the address of instruction to be executed
>>> > next after the kprobed instruction. But regs->eip in post_handler()
>>> > contains value which is useless to the user. It's pre-corrected value.
>>> > This value is difficult to use without access to resume_execution(), which
>>> > is not exported anyway.
>>> > I moved the invocation of post_handler() to *after* resume_execution().
>>> > Now regs->eip contains meaningful value in post_handler().
>>> >
>>> > I do not think this change breaks any backward-compatibility.
>>> > To make meaning of the old value, post_handler() would need access to
>>> > resume_execution() which is not exported. I have difficulty to believe
>>> > that previous, uncorrected, regs->eip can be meaningfully used in
>>> > post_handler().
>>>
>>> resume_execution() exists not just for the program counter fixups after
>>> out-of-line singlestepping, but is also as an insurance to put the
>>> program counter back to the correct address in case the user's
>>> post_handler() mucks around with it. That isn't possible with this
>>> change :-(
>> I see your point. This can be prevented by saving and restoring regs->ip
>> around the post_handler() call, no ? Current code is beautiful. Saving and
>> restoring regs->ip would make this place look ugly.
>>
>> Otoh, if the post_handler() wants to crash the kernel, it can do it
>> in thousand ways, not just by trashing regs->ip, no ?
>
> Of course, there still are other ways to shoot yourself in the foot with
> the post_handler(), but, atleast for cases we can control, we need to do
> the right thing.
Ananth, I think we can not prevent it even if resume_execution() is called
after post_handler, because resume_execution() refers reg->ip...:-(
And Yakov, I think you might need to make a patchset against all arch which
support kprobes, because this patch modifies expected behavior of kprobes
only on x86.
IMHO, Yakov's suggestion will be also good for resume_execution(), because
it only has to clean up after expectable-single-stepping. (user code is
unexpectable... we can not control all of that)
Thanks,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected]
On Mon, Mar 17, 2008 at 06:17:21PM -0400, Masami Hiramatsu wrote:
> Ananth N Mavinakayanahalli wrote:
> > On Mon, Mar 17, 2008 at 12:59:05PM +0200, Yakov Lerner wrote:
> >> On Mon, Mar 17, 2008 at 7:19 AM, Ananth N Mavinakayanahalli
> >> <[email protected]> wrote:
> >>> On Sun, Mar 16, 2008 at 03:21:21AM -0500, Yakov Lerner wrote:
> >>> >
> >>> > I was trying to get the address of instruction to be executed
> >>> > next after the kprobed instruction. But regs->eip in post_handler()
> >>> > contains value which is useless to the user. It's pre-corrected value.
> >>> > This value is difficult to use without access to resume_execution(), which
> >>> > is not exported anyway.
> >>> > I moved the invocation of post_handler() to *after* resume_execution().
> >>> > Now regs->eip contains meaningful value in post_handler().
> >>> >
> >>> > I do not think this change breaks any backward-compatibility.
> >>> > To make meaning of the old value, post_handler() would need access to
> >>> > resume_execution() which is not exported. I have difficulty to believe
> >>> > that previous, uncorrected, regs->eip can be meaningfully used in
> >>> > post_handler().
> >>>
> >>> resume_execution() exists not just for the program counter fixups after
> >>> out-of-line singlestepping, but is also as an insurance to put the
> >>> program counter back to the correct address in case the user's
> >>> post_handler() mucks around with it. That isn't possible with this
> >>> change :-(
> >> I see your point. This can be prevented by saving and restoring regs->ip
> >> around the post_handler() call, no ? Current code is beautiful. Saving and
> >> restoring regs->ip would make this place look ugly.
> >>
> >> Otoh, if the post_handler() wants to crash the kernel, it can do it
> >> in thousand ways, not just by trashing regs->ip, no ?
> >
> > Of course, there still are other ways to shoot yourself in the foot with
> > the post_handler(), but, atleast for cases we can control, we need to do
> > the right thing.
>
> Ananth, I think we can not prevent it even if resume_execution() is called
> after post_handler, because resume_execution() refers reg->ip...:-(
OK, I see what you are referring to... though we use kp.addr and
kp.ainsn.insn to start with, we'll need the knowledge of regs->ip at the
end.
> And Yakov, I think you might need to make a patchset against all arch which
> support kprobes, because this patch modifies expected behavior of kprobes
> only on x86.
Agreed.
> IMHO, Yakov's suggestion will be also good for resume_execution(), because
> it only has to clean up after expectable-single-stepping. (user code is
> unexpectable... we can not control all of that)
Ananth
* Yakov Lerner <[email protected]> wrote:
> I was trying to get the address of instruction to be executed next
> after the kprobed instruction. But regs->eip in post_handler()
> contains value which is useless to the user. It's pre-corrected value.
> This value is difficult to use without access to resume_execution(),
> which is not exported anyway. I moved the invocation of post_handler()
> to *after* resume_execution(). Now regs->eip contains meaningful value
> in post_handler().
>
> I do not think this change breaks any backward-compatibility. To make
> meaning of the old value, post_handler() would need access to
> resume_execution() which is not exported. I have difficulty to
> believe that previous, uncorrected, regs->eip can be meaningfully used
> in post_handler().
thanks, i've added your patch to the .26 bucket of x86.git, but it would
be nice to get an Ack/Nack from a kprobes person as well.
Ingo
On Fri, Mar 21, 2008 at 12:08:04PM +0100, Ingo Molnar wrote:
>
> * Yakov Lerner <[email protected]> wrote:
>
> > I was trying to get the address of instruction to be executed next
> > after the kprobed instruction. But regs->eip in post_handler()
> > contains value which is useless to the user. It's pre-corrected value.
> > This value is difficult to use without access to resume_execution(),
> > which is not exported anyway. I moved the invocation of post_handler()
> > to *after* resume_execution(). Now regs->eip contains meaningful value
> > in post_handler().
> >
> > I do not think this change breaks any backward-compatibility. To make
> > meaning of the old value, post_handler() would need access to
> > resume_execution() which is not exported. I have difficulty to
> > believe that previous, uncorrected, regs->eip can be meaningfully used
> > in post_handler().
>
> thanks, i've added your patch to the .26 bucket of x86.git, but it would
> be nice to get an Ack/Nack from a kprobes person as well.
Ingo,
I've tested Yakov's more comprehensive patch on powerpc too. This has my
ack.
Acked-by: Ananth N Mavinakayanahalli <[email protected]>
* Ananth N Mavinakayanahalli <[email protected]> wrote:
> > thanks, i've added your patch to the .26 bucket of x86.git, but it
> > would be nice to get an Ack/Nack from a kprobes person as well.
>
> Ingo,
>
> I've tested Yakov's more comprehensive patch on powerpc too. This has
> my ack.
>
> Acked-by: Ananth N Mavinakayanahalli <[email protected]>
thanks, i've queued up the x86-only patch below for .26 merging. (that
is all that is needed for x86, and no .25 urgency, right?)
Ingo
------------------>
Subject: x86, kprobes: correct post-eip value in post_hander()
From: "Yakov Lerner" <[email protected]>
Date: Sun, 16 Mar 2008 03:21:21 -0500
I was trying to get the address of instruction to be executed
next after the kprobed instruction. But regs->eip in post_handler()
contains value which is useless to the user. It's pre-corrected value.
This value is difficult to use without access to resume_execution(), which
is not exported anyway.
I moved the invocation of post_handler() to *after* resume_execution().
Now regs->eip contains meaningful value in post_handler().
I do not think this change breaks any backward-compatibility.
To make meaning of the old value, post_handler() would need access to
resume_execution() which is not exported. I have difficulty to believe
that previous, uncorrected, regs->eip can be meaningfully used in
post_handler().
Signed-off-by: Yakov Lerner <[email protected]>
Acked-by: Ananth N Mavinakayanahalli <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/kprobes.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Index: linux-x86.q/arch/x86/kernel/kprobes.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/kprobes.c
+++ linux-x86.q/arch/x86/kernel/kprobes.c
@@ -858,15 +858,15 @@ static int __kprobes post_kprobe_handler
if (!cur)
return 0;
+ resume_execution(cur, regs, kcb);
+ regs->flags |= kcb->kprobe_saved_flags;
+ trace_hardirqs_fixup_flags(regs->flags);
+
if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
kcb->kprobe_status = KPROBE_HIT_SSDONE;
cur->post_handler(cur, regs, 0);
}
- resume_execution(cur, regs, kcb);
- regs->flags |= kcb->kprobe_saved_flags;
- trace_hardirqs_fixup_flags(regs->flags);
-
/* Restore back the original saved kprobes variables and continue. */
if (kcb->kprobe_status == KPROBE_REENTER) {
restore_previous_kprobe(kcb);
On Fri, Mar 21, 2008 at 03:32:29PM +0100, Ingo Molnar wrote:
>
> * Ananth N Mavinakayanahalli <[email protected]> wrote:
>
> > > thanks, i've added your patch to the .26 bucket of x86.git, but it
> > > would be nice to get an Ack/Nack from a kprobes person as well.
> >
> > Ingo,
> >
> > I've tested Yakov's more comprehensive patch on powerpc too. This has
> > my ack.
> >
> > Acked-by: Ananth N Mavinakayanahalli <[email protected]>
>
> thanks, i've queued up the x86-only patch below for .26 merging. (that
> is all that is needed for x86, and no .25 urgency, right?)
Yup. This can wait for .26
Ananth
Ingo Molnar wrote:
> * Yakov Lerner <[email protected]> wrote:
>
>> I was trying to get the address of instruction to be executed next
>> after the kprobed instruction. But regs->eip in post_handler()
>> contains value which is useless to the user. It's pre-corrected value.
>> This value is difficult to use without access to resume_execution(),
>> which is not exported anyway. I moved the invocation of post_handler()
>> to *after* resume_execution(). Now regs->eip contains meaningful value
>> in post_handler().
>>
>> I do not think this change breaks any backward-compatibility. To make
>> meaning of the old value, post_handler() would need access to
>> resume_execution() which is not exported. I have difficulty to
>> believe that previous, uncorrected, regs->eip can be meaningfully used
>> in post_handler().
>
> thanks, i've added your patch to the .26 bucket of x86.git, but it would
> be nice to get an Ack/Nack from a kprobes person as well.
>
> Ingo
Ingo, I also tested this on x86-64/x86/ia64.
Acked-by: Masami Hiramatsu <[email protected]>
Thanks,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected]