Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
Jordan Niethe's on February 28, 2020 1:23 pm:
> On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin wrote:
>>
>> Jordan Niethe's on February 27, 2020 10:58 am:
>> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> >> > }
>> >> >
>> >> > if (!ret) {
>> >> > - patch_instruction(p->ainsn.insn, *p->addr);
>> >> > + patch_instruction(>ainsn.insn[0], p->addr[0]);
>> >> > + if (IS_PREFIX(insn))
>> >> > + patch_instruction(>ainsn.insn[1], p->addr[1]);
>> >> > p->opcode = *p->addr;
>> >>
>> >> Not to single out this hunk or this patch even, but what do you reckon
>> >> about adding an instruction data type, and then use that in all these
>> >> call sites rather than adding the extra arg or doing the extra copy
>> >> manually in each place depending on prefix?
>> >>
>> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> >> etc., would all take this new instr. Places that open code a memory
>> >> access like your MCE change need some accessor
>> >>
>> >>instr = *(unsigned int *)(instr_addr);
>> >> - if (!analyse_instr(, , instr, PPC_NO_SUFFIX)) {
>> >> + if (IS_PREFIX(instr))
>> >> + suffix = *(unsigned int *)(instr_addr + 4);
>> >>
>> >> Becomes
>> >>read_instr(instr_addr, );
>> >>if (!analyse_instr(, , instr)) ...
>> >>
>> >> etc.
>> > Daniel Axtens also talked about this and my reasons not to do so were
>> > pretty unconvincing, so I started trying something like this. One
>>
>> Okay.
>>
>> > thing I have been wondering is how pervasive should the new type be.
>>
>> I wouldn't mind it being quite pervasive. We have to be careful not
>> to copy it directly to/from memory now, but if we have accessors to
>> do all that with, I think it should be okay.
>>
>> > Below is data type I have started using, which I think works
>> > reasonably for replacing unsigned ints everywhere (like within
>> > code-patching.c). In a few architecture independent places such as
>> > uprobes which want to do ==, etc the union type does not work so well.
>>
>> There will be some places you have to make the boundary. I would start
>> by just making it a powerpc thing, but possibly there is or could be
>> some generic helpers. How does something like x86 cope with this?
>
> One of the places I was thinking of was is_swbp_insn() in
> kernel/events/uprobes.c. The problem was I wanted to typedef
> uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
> do. x86 typedef's it as u8 (size of the trap they use). So we probably
> can do the same thing and just keep it as a u32.
Sounds good.
>> > I will have the next revision of the series start using a type.
>>
>> Thanks for doing that.
>>
>> >
>> > diff --git a/arch/powerpc/include/asm/inst.h
>> > b/arch/powerpc/include/asm/inst.h
>> > new file mode 100644
>> > index ..50adb3dbdeb4
>> > --- /dev/null
>> > +++ b/arch/powerpc/include/asm/inst.h
>> > @@ -0,0 +1,87 @@
>> > +
>> > +#ifndef _ASM_INST_H
>> > +#define _ASM_INST_H
>> > +
>> > +#ifdef __powerpc64__
>> > +
>> > +/* 64 bit Instruction */
>> > +
>> > +typedef struct {
>> > +unsigned int prefix;
>> > +unsigned int suffix;
>>
>> u32?
> Sure.
>>
>> > +} __packed ppc_prefixed_inst;
>> > +
>> > +typedef union ppc_inst {
>> > +unsigned int w;
>> > +ppc_prefixed_inst p;
>> > +} ppc_inst;
>>
>> I'd make it a struct and use nameless structs/unions inside it (with
>> appropriate packed annotation):
> Yeah that will be nicer.
>>
>> struct ppc_inst {
>> union {
>> struct {
>> u32 word;
>> u32 pad;
>> };
>> struct {
>> u32 prefix;
>> u32 suffix;
>> };
>> };
>> };
>>
>> > +
>> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
>> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
>> > sizeof((inst).p) : sizeof((inst).w))
>>
>> Good accessors, I'd make them all C inline functions and lower case.
> Will change.
>>
>> > +
>> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
>> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (0x6000) })
>> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (y) })
>>
>> If these are widely used, I'd make them a bit shorter
>>
>> #define PPC_INST(x)
>> #define PPC_INST_PREFIXED(x)
> Good idea, they do end up used quite widely.
>>
>> I'd also set padding to something invalid 0 or 0x for the first
>> case, and have your accessors check that rather than carrying around
>> another type of ppc_inst (prefixed, padded, non-padded).
>>
>> > +
>> > +#define PPC_INST_WORD(x) ((x).w)
>> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
>> > +#define PPC_INST_SUFFIX(x)
Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin wrote:
>
> Jordan Niethe's on February 27, 2020 10:58 am:
> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin wrote:
> >>
> >> Jordan Niethe's on February 26, 2020 2:07 pm:
> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> >> > }
> >> >
> >> > if (!ret) {
> >> > - patch_instruction(p->ainsn.insn, *p->addr);
> >> > + patch_instruction(>ainsn.insn[0], p->addr[0]);
> >> > + if (IS_PREFIX(insn))
> >> > + patch_instruction(>ainsn.insn[1], p->addr[1]);
> >> > p->opcode = *p->addr;
> >>
> >> Not to single out this hunk or this patch even, but what do you reckon
> >> about adding an instruction data type, and then use that in all these
> >> call sites rather than adding the extra arg or doing the extra copy
> >> manually in each place depending on prefix?
> >>
> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
> >> etc., would all take this new instr. Places that open code a memory
> >> access like your MCE change need some accessor
> >>
> >>instr = *(unsigned int *)(instr_addr);
> >> - if (!analyse_instr(, , instr, PPC_NO_SUFFIX)) {
> >> + if (IS_PREFIX(instr))
> >> + suffix = *(unsigned int *)(instr_addr + 4);
> >>
> >> Becomes
> >>read_instr(instr_addr, );
> >>if (!analyse_instr(, , instr)) ...
> >>
> >> etc.
> > Daniel Axtens also talked about this and my reasons not to do so were
> > pretty unconvincing, so I started trying something like this. One
>
> Okay.
>
> > thing I have been wondering is how pervasive should the new type be.
>
> I wouldn't mind it being quite pervasive. We have to be careful not
> to copy it directly to/from memory now, but if we have accessors to
> do all that with, I think it should be okay.
>
> > Below is data type I have started using, which I think works
> > reasonably for replacing unsigned ints everywhere (like within
> > code-patching.c). In a few architecture independent places such as
> > uprobes which want to do ==, etc the union type does not work so well.
>
> There will be some places you have to make the boundary. I would start
> by just making it a powerpc thing, but possibly there is or could be
> some generic helpers. How does something like x86 cope with this?
One of the places I was thinking of was is_swbp_insn() in
kernel/events/uprobes.c. The problem was I wanted to typedef
uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
do. x86 typedef's it as u8 (size of the trap they use). So we probably
can do the same thing and just keep it as a u32.
>
> > I will have the next revision of the series start using a type.
>
> Thanks for doing that.
>
> >
> > diff --git a/arch/powerpc/include/asm/inst.h
> > b/arch/powerpc/include/asm/inst.h
> > new file mode 100644
> > index ..50adb3dbdeb4
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -0,0 +1,87 @@
> > +
> > +#ifndef _ASM_INST_H
> > +#define _ASM_INST_H
> > +
> > +#ifdef __powerpc64__
> > +
> > +/* 64 bit Instruction */
> > +
> > +typedef struct {
> > +unsigned int prefix;
> > +unsigned int suffix;
>
> u32?
Sure.
>
> > +} __packed ppc_prefixed_inst;
> > +
> > +typedef union ppc_inst {
> > +unsigned int w;
> > +ppc_prefixed_inst p;
> > +} ppc_inst;
>
> I'd make it a struct and use nameless structs/unions inside it (with
> appropriate packed annotation):
Yeah that will be nicer.
>
> struct ppc_inst {
> union {
> struct {
> u32 word;
> u32 pad;
> };
> struct {
> u32 prefix;
> u32 suffix;
> };
> };
> };
>
> > +
> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
> > sizeof((inst).p) : sizeof((inst).w))
>
> Good accessors, I'd make them all C inline functions and lower case.
Will change.
>
> > +
> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
> > .p.suffix = (0x6000) })
> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
> > .p.suffix = (y) })
>
> If these are widely used, I'd make them a bit shorter
>
> #define PPC_INST(x)
> #define PPC_INST_PREFIXED(x)
Good idea, they do end up used quite widely.
>
> I'd also set padding to something invalid 0 or 0x for the first
> case, and have your accessors check that rather than carrying around
> another type of ppc_inst (prefixed, padded, non-padded).
>
> > +
> > +#define PPC_INST_WORD(x) ((x).w)
> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
> > +#define PPC_INST_SUFFIX(x) (x.p.suffix)
> > +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
>
> I'd avoid simple accessors like this completely. If they have any use
> it would be to ensure you don't try to use prefix/suffix on a
>
Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
Nicholas Piggin's on February 28, 2020 11:47 am:
> Jordan Niethe's on February 27, 2020 10:58 am:
>> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin wrote:
>> +
>> +#define DEREF_PPC_INST_PTR(ptr)\
>> +({\
>> +ppc_inst __inst;\
>> +__inst.w = *(unsigned int *)(ptr);\
>> +if (PPC_INST_IS_PREFIXED(__inst))\
>> +__inst.p = *(ppc_prefixed_inst *)(ptr);\
>> +__inst;\
>> +})
>
> I'd go an inline with shorter lowercase names. ppc_inst_read();
Probably inst = ppc_inst_read(mem), rather.
Thanks,
Nick
Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
Jordan Niethe's on February 27, 2020 10:58 am:
> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> > }
>> >
>> > if (!ret) {
>> > - patch_instruction(p->ainsn.insn, *p->addr);
>> > + patch_instruction(>ainsn.insn[0], p->addr[0]);
>> > + if (IS_PREFIX(insn))
>> > + patch_instruction(>ainsn.insn[1], p->addr[1]);
>> > p->opcode = *p->addr;
>>
>> Not to single out this hunk or this patch even, but what do you reckon
>> about adding an instruction data type, and then use that in all these
>> call sites rather than adding the extra arg or doing the extra copy
>> manually in each place depending on prefix?
>>
>> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> etc., would all take this new instr. Places that open code a memory
>> access like your MCE change need some accessor
>>
>>instr = *(unsigned int *)(instr_addr);
>> - if (!analyse_instr(, , instr, PPC_NO_SUFFIX)) {
>> + if (IS_PREFIX(instr))
>> + suffix = *(unsigned int *)(instr_addr + 4);
>>
>> Becomes
>>read_instr(instr_addr, );
>>if (!analyse_instr(, , instr)) ...
>>
>> etc.
> Daniel Axtens also talked about this and my reasons not to do so were
> pretty unconvincing, so I started trying something like this. One
Okay.
> thing I have been wondering is how pervasive should the new type be.
I wouldn't mind it being quite pervasive. We have to be careful not
to copy it directly to/from memory now, but if we have accessors to
do all that with, I think it should be okay.
> Below is data type I have started using, which I think works
> reasonably for replacing unsigned ints everywhere (like within
> code-patching.c). In a few architecture independent places such as
> uprobes which want to do ==, etc the union type does not work so well.
There will be some places you have to make the boundary. I would start
by just making it a powerpc thing, but possibly there is or could be
some generic helpers. How does something like x86 cope with this?
> I will have the next revision of the series start using a type.
Thanks for doing that.
>
> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> new file mode 100644
> index ..50adb3dbdeb4
> --- /dev/null
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -0,0 +1,87 @@
> +
> +#ifndef _ASM_INST_H
> +#define _ASM_INST_H
> +
> +#ifdef __powerpc64__
> +
> +/* 64 bit Instruction */
> +
> +typedef struct {
> +unsigned int prefix;
> +unsigned int suffix;
u32?
> +} __packed ppc_prefixed_inst;
> +
> +typedef union ppc_inst {
> +unsigned int w;
> +ppc_prefixed_inst p;
> +} ppc_inst;
I'd make it a struct and use nameless structs/unions inside it (with
appropriate packed annotation):
struct ppc_inst {
union {
struct {
u32 word;
u32 pad;
};
struct {
u32 prefix;
u32 suffix;
};
};
};
> +
> +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
> +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
> sizeof((inst).p) : sizeof((inst).w))
Good accessors, I'd make them all C inline functions and lower case.
> +
> +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
> +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
> .p.suffix = (0x6000) })
> +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
> .p.suffix = (y) })
If these are widely used, I'd make them a bit shorter
#define PPC_INST(x)
#define PPC_INST_PREFIXED(x)
I'd also set padding to something invalid 0 or 0x for the first
case, and have your accessors check that rather than carrying around
another type of ppc_inst (prefixed, padded, non-padded).
> +
> +#define PPC_INST_WORD(x) ((x).w)
> +#define PPC_INST_PREFIX(x) (x.p.prefix)
> +#define PPC_INST_SUFFIX(x) (x.p.suffix)
> +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
I'd avoid simple accessors like this completely. If they have any use
it would be to ensure you don't try to use prefix/suffix on a
non-prefixed instruction for example. If you want to do that I'd use
inline functions for it.
> +
> +#define DEREF_PPC_INST_PTR(ptr)\
> +({\
> +ppc_inst __inst;\
> +__inst.w = *(unsigned int *)(ptr);\
> +if (PPC_INST_IS_PREFIXED(__inst))\
> +__inst.p = *(ppc_prefixed_inst *)(ptr);\
> +__inst;\
> +})
I'd go an inline with shorter lowercase names. ppc_inst_read();
> +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr
> +#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr
Wouldn't bother with this accessor, caller could
Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> > }
> >
> > if (!ret) {
> > - patch_instruction(p->ainsn.insn, *p->addr);
> > + patch_instruction(>ainsn.insn[0], p->addr[0]);
> > + if (IS_PREFIX(insn))
> > + patch_instruction(>ainsn.insn[1], p->addr[1]);
> > p->opcode = *p->addr;
>
> Not to single out this hunk or this patch even, but what do you reckon
> about adding an instruction data type, and then use that in all these
> call sites rather than adding the extra arg or doing the extra copy
> manually in each place depending on prefix?
>
> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
> etc., would all take this new instr. Places that open code a memory
> access like your MCE change need some accessor
>
>instr = *(unsigned int *)(instr_addr);
> - if (!analyse_instr(, , instr, PPC_NO_SUFFIX)) {
> + if (IS_PREFIX(instr))
> + suffix = *(unsigned int *)(instr_addr + 4);
>
> Becomes
>read_instr(instr_addr, );
>if (!analyse_instr(, , instr)) ...
>
> etc.
Daniel Axtens also talked about this and my reasons not to do so were
pretty unconvincing, so I started trying something like this. One
thing I have been wondering is how pervasive should the new type be.
Below is data type I have started using, which I think works
reasonably for replacing unsigned ints everywhere (like within
code-patching.c). In a few architecture independent places such as
uprobes which want to do ==, etc the union type does not work so well.
I will have the next revision of the series start using a type.
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
new file mode 100644
index ..50adb3dbdeb4
--- /dev/null
+++ b/arch/powerpc/include/asm/inst.h
@@ -0,0 +1,87 @@
+
+#ifndef _ASM_INST_H
+#define _ASM_INST_H
+
+#ifdef __powerpc64__
+
+/* 64 bit Instruction */
+
+typedef struct {
+unsigned int prefix;
+unsigned int suffix;
+} __packed ppc_prefixed_inst;
+
+typedef union ppc_inst {
+unsigned int w;
+ppc_prefixed_inst p;
+} ppc_inst;
+
+#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
+#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
sizeof((inst).p) : sizeof((inst).w))
+
+#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
+#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
.p.suffix = (0x6000) })
+#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
.p.suffix = (y) })
+
+#define PPC_INST_WORD(x) ((x).w)
+#define PPC_INST_PREFIX(x) (x.p.prefix)
+#define PPC_INST_SUFFIX(x) (x.p.suffix)
+#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
+
+#define DEREF_PPC_INST_PTR(ptr)\
+({\
+ppc_inst __inst;\
+__inst.w = *(unsigned int *)(ptr);\
+if (PPC_INST_IS_PREFIXED(__inst))\
+__inst.p = *(ppc_prefixed_inst *)(ptr);\
+__inst;\
+})
+
+#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr
+#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr
+
+#define PPC_INST_EQ(x, y)\
+({\
+long pic_ret = 0;\
+pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y));\
+if (pic_ret) {\
+if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) {\
+pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y));\
+} else {\
+pic_ret = 0;\
+}\
+}\
+pic_ret;\
+})
+
+#else /* !__powerpc64__ */
+
+/* 32 bit Instruction */
+
+typedef unsigned int ppc_inst;
+
+#define PPC_INST_IS_PREFIXED(inst) (0)
+#define PPC_INST_LEN(inst) (4)
+
+#define PPC_INST_NEW_WORD(x) (x)
+#define PPC_INST_NEW_WORD_PAD(x) (x)
+#define PPC_INST_NEW_PREFIXED(x, y) (x)
+
+#define PPC_INST_WORD(x) (x)
+#define PPC_INST_PREFIX(x) (x)
+#define PPC_INST_SUFFIX(x) (0)
+#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
+
+#define DEREF_PPC_INST_PTR(ptr)(*ptr)
+
+#define PPC_INST_NEXT(ptr) ((ptr) += 4)
+#define PPC_INST_PREV(ptr) ((ptr) -= 4)
+
+#define PPC_INST_EQ(x, y) ((x) == (y))
+
+#endif /* __powerpc64__ */
+
+
+#endif /* _ASM_INST_H */
>
> Thanks,
> Nick
Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
Jordan Niethe's on February 26, 2020 2:07 pm:
> @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> }
>
> if (!ret) {
> - patch_instruction(p->ainsn.insn, *p->addr);
> + patch_instruction(>ainsn.insn[0], p->addr[0]);
> + if (IS_PREFIX(insn))
> + patch_instruction(>ainsn.insn[1], p->addr[1]);
> p->opcode = *p->addr;
Not to single out this hunk or this patch even, but what do you reckon
about adding an instruction data type, and then use that in all these
call sites rather than adding the extra arg or doing the extra copy
manually in each place depending on prefix?
instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
etc., would all take this new instr. Places that open code a memory
access like your MCE change need some accessor
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(, , instr, PPC_NO_SUFFIX)) {
+ if (IS_PREFIX(instr))
+ suffix = *(unsigned int *)(instr_addr + 4);
Becomes
read_instr(instr_addr, );
if (!analyse_instr(, , instr)) ...
etc.
Thanks,
Nick
[PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
A prefixed instruction is composed of a word prefix followed by a word
suffix. It does not make sense to be able to have a kprobe on the suffix
of a prefixed instruction, so make this impossible.
Kprobes work by replacing an instruction with a trap and saving that
instruction to be single stepped out of place later. Currently there is
not enough space allocated to keep a prefixed instruction for single
stepping. Increase the amount of space allocated for holding the
instruction copy.
kprobe_post_handler() expects all instructions to be 4 bytes long which
means that it does not function correctly for prefixed instructions.
Add checks for prefixed instructions which will use a length of 8 bytes
instead.
For optprobes we normally patch in loading the instruction we put a
probe on into r4 before calling emulate_step(). We now make space and
patch in loading the suffix into r5 as well.
Signed-off-by: Jordan Niethe
---
v3: - Base on top of https://patchwork.ozlabs.org/patch/1232619/
- Change printing format to %x:%x
---
arch/powerpc/include/asm/kprobes.h | 5 ++--
arch/powerpc/kernel/kprobes.c| 43 +---
arch/powerpc/kernel/optprobes.c | 32 -
arch/powerpc/kernel/optprobes_head.S | 6
4 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/kprobes.h
b/arch/powerpc/include/asm/kprobes.h
index 66b3f2983b22..0d44ce8a3163 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
extern kprobe_opcode_t optprobe_template_op_address[];
extern kprobe_opcode_t optprobe_template_call_handler[];
extern kprobe_opcode_t optprobe_template_insn[];
+extern kprobe_opcode_t optprobe_template_suffix[];
extern kprobe_opcode_t optprobe_template_call_emulate[];
extern kprobe_opcode_t optprobe_template_ret[];
extern kprobe_opcode_t optprobe_template_end[];
-/* Fixed instruction size for powerpc */
-#define MAX_INSN_SIZE 1
+/* Prefixed instructions are two words */
+#define MAX_INSN_SIZE 2
#define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
#define MAX_OPTINSN_SIZE (optprobe_template_end -
optprobe_template_entry)
#define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 6b2e9e37f12b..9ccf1b9a1275 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -117,16 +117,28 @@ void *alloc_insn_page(void)
int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
+ struct kprobe *prev;
kprobe_opcode_t insn = *p->addr;
+ kprobe_opcode_t prefix = *(p->addr - 1);
+ preempt_disable();
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
ret = -EINVAL;
} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
ret = -EINVAL;
+ } else if (IS_PREFIX(prefix)) {
+ printk("Cannot register a kprobe on the second word of prefixed
instruction\n");
+ ret = -EINVAL;
+ }
+ prev = get_kprobe(p->addr - 1);
+ if (prev && IS_PREFIX(*prev->ainsn.insn)) {
+ printk("Cannot register a kprobe on the second word of prefixed
instruction\n");
+ ret = -EINVAL;
}
+
/* insn must be on a special executable page on ppc64. This is
* not explicitly required on ppc32 (right now), but it doesn't hurt */
if (!ret) {
@@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- patch_instruction(p->ainsn.insn, *p->addr);
+ patch_instruction(>ainsn.insn[0], p->addr[0]);
+ if (IS_PREFIX(insn))
+ patch_instruction(>ainsn.insn[1], p->addr[1]);
p->opcode = *p->addr;
}
p->ainsn.boostable = 0;
+ preempt_enable_no_resched();
return ret;
}
NOKPROBE_SYMBOL(arch_prepare_kprobe);
@@ -225,10 +240,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
{
int ret;
- unsigned int insn = *p->ainsn.insn;
+ unsigned int insn = p->ainsn.insn[0];
+ unsigned int suffix = p->ainsn.insn[1];
/* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, insn, suffix);
if (ret > 0) {
/*
* Once this instruction has been boosted
@@ -242,7 +258,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs
*regs)
* So, we should never get here... but, its still
* good to catch them, just in
