| Message ID | 1387166930-13182-14-git-send-email-dave.long@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2013-12-15 at 23:08 -0500, David Long wrote: > From: "David A. Long" <dave.long@linaro.org> > > Add an emulate flag into the instruction interpreter, primarily for uprobes > support. > > Signed-off-by: David A. Long <dave.long@linaro.org> > --- > arch/arm/kernel/kprobes.c | 3 ++- > arch/arm/kernel/kprobes.h | 1 + > arch/arm/kernel/probes-arm.c | 4 ++-- > arch/arm/kernel/probes-arm.h | 2 +- > arch/arm/kernel/probes-thumb.c | 8 ++++---- > arch/arm/kernel/probes-thumb.h | 4 ++-- > arch/arm/kernel/probes.c | 32 +++++++++++++++++++++++--------- > arch/arm/kernel/probes.h | 2 +- > 8 files changed, 36 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c > index 0d9d49b..04690f9 100644 > --- a/arch/arm/kernel/kprobes.c > +++ b/arch/arm/kernel/kprobes.c > @@ -87,7 +87,8 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) > p->opcode = insn; > p->ainsn.insn = tmp_insn; > > - switch ((*decode_insn)(insn, &p->ainsn, actions)) { > + switch ((*decode_insn)(insn, &p->ainsn, > + true, actions)) { Any reason why the function args need splitting over two lines? > case INSN_REJECTED: /* not supported */ > return -EINVAL; > [...] > --- a/arch/arm/kernel/probes.c > +++ b/arch/arm/kernel/probes.c > @@ -193,7 +193,7 @@ void __kprobes probes_emulate_none(probes_opcode_t opcode, > */ > static probes_opcode_t __kprobes > prepare_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > - bool thumb) > + bool thumb) Seems like a spurious indentation change. > { > #ifdef CONFIG_THUMB2_KERNEL > if (thumb) { > @@ -218,7 +218,7 @@ prepare_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > */ > static void __kprobes > set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > - bool thumb) > + bool thumb) Another spurious whitespace change. > { > #ifdef CONFIG_THUMB2_KERNEL > if (thumb) { > @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > * non-zero value, the corresponding nibble in pinsn is validated and modified > * according to the type. > */ > -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) > +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify) > { > probes_opcode_t insn = *pinsn; > probes_opcode_t mask = 0xf; /* Start at least significant nibble */ > @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) > /* Replace value of nibble with new register number... */ > insn &= ~mask; > insn |= new_bits & mask; > + if (modify) { > + /* Replace value of nibble with new register number */ > + insn &= ~mask; > + insn |= new_bits & mask; > + } Huh? As is, the above addition doesn't do anything because insn has already been modified. I guess you played with the idea that you needed to avoid changing insn (you don't) and then didn't undo the experiment quite right. :-) > } > > - *pinsn = insn; > + if (modify) > + *pinsn = insn; > + > return true; > > reject: > @@ -380,14 +387,15 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = { > */ > int __kprobes > probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > - const union decode_item *table, bool thumb, > - const union decode_item *actions) > + const union decode_item *table, bool thumb, > + bool emulate, const union decode_item *actions) > { > struct decode_header *h = (struct decode_header *)table; > struct decode_header *next; > bool matched = false; > > - insn = prepare_emulated_insn(insn, asi, thumb); > + if (emulate) > + insn = prepare_emulated_insn(insn, asi, thumb); > > for (;; h = next) { > enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK; > @@ -402,7 +410,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > if (!matched && (insn & h->mask.bits) != h->value.bits) > continue; > > - if (!decode_regs(&insn, regs)) > + if (!decode_regs(&insn, regs, emulate)) > return INSN_REJECTED; > > switch (type) { > @@ -415,7 +423,8 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > > case DECODE_TYPE_CUSTOM: { > struct decode_custom *d = (struct decode_custom *)h; > - return actions[d->decoder.bits].decoder(insn, asi, h); > + return actions[d->decoder.bits].decoder(insn, > + asi, h); No need to split the above line, you haven't changed it and it doesn't exceed 80 characters anyway. [Rest of patch cut]
On 12/20/13 09:58, Jon Medhurst (Tixy) wrote: > On Sun, 2013-12-15 at 23:08 -0500, David Long wrote: >> From: "David A. Long" <dave.long@linaro.org> >> >> Add an emulate flag into the instruction interpreter, primarily for uprobes >> support. >> >> Signed-off-by: David A. Long <dave.long@linaro.org> >> --- >> arch/arm/kernel/kprobes.c | 3 ++- >> arch/arm/kernel/kprobes.h | 1 + >> arch/arm/kernel/probes-arm.c | 4 ++-- >> arch/arm/kernel/probes-arm.h | 2 +- >> arch/arm/kernel/probes-thumb.c | 8 ++++---- >> arch/arm/kernel/probes-thumb.h | 4 ++-- >> arch/arm/kernel/probes.c | 32 +++++++++++++++++++++++--------- >> arch/arm/kernel/probes.h | 2 +- >> 8 files changed, 36 insertions(+), 20 deletions(-) >> >> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c >> index 0d9d49b..04690f9 100644 >> --- a/arch/arm/kernel/kprobes.c >> +++ b/arch/arm/kernel/kprobes.c >> @@ -87,7 +87,8 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) >> p->opcode = insn; >> p->ainsn.insn = tmp_insn; >> >> - switch ((*decode_insn)(insn, &p->ainsn, actions)) { >> + switch ((*decode_insn)(insn, &p->ainsn, >> + true, actions)) { > > Any reason why the function args need splitting over two lines? I undid the that change. >> case INSN_REJECTED: /* not supported */ >> return -EINVAL; >> > > [...] > >> --- a/arch/arm/kernel/probes.c >> +++ b/arch/arm/kernel/probes.c >> @@ -193,7 +193,7 @@ void __kprobes probes_emulate_none(probes_opcode_t opcode, >> */ >> static probes_opcode_t __kprobes >> prepare_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> - bool thumb) >> + bool thumb) > > Seems like a spurious indentation change. Fixed. >> { >> #ifdef CONFIG_THUMB2_KERNEL >> if (thumb) { >> @@ -218,7 +218,7 @@ prepare_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> */ >> static void __kprobes >> set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> - bool thumb) >> + bool thumb) > > Another spurious whitespace change. Fixed. >> { >> #ifdef CONFIG_THUMB2_KERNEL >> if (thumb) { >> @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> * non-zero value, the corresponding nibble in pinsn is validated and modified >> * according to the type. >> */ >> -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) >> +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify) >> { >> probes_opcode_t insn = *pinsn; >> probes_opcode_t mask = 0xf; /* Start at least significant nibble */ >> @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) >> /* Replace value of nibble with new register number... */ >> insn &= ~mask; >> insn |= new_bits & mask; >> + if (modify) { >> + /* Replace value of nibble with new register number */ >> + insn &= ~mask; >> + insn |= new_bits & mask; >> + } > > Huh? As is, the above addition doesn't do anything because insn has > already been modified. I guess you played with the idea that you needed > to avoid changing insn (you don't) and then didn't undo the experiment > quite right. :-) > The conditional modification of the instruction was part of Rabin's original work for uprobes, but I messed up the merge from an earlier working version of my patches. My intention was/is to delete the old unconditional code. Sounds like maybe you disagree though. The intent is to only modify the instruction in the kprobes case. >> } >> >> - *pinsn = insn; >> + if (modify) >> + *pinsn = insn; >> + >> return true; >> >> reject: >> @@ -380,14 +387,15 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = { >> */ >> int __kprobes >> probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> - const union decode_item *table, bool thumb, >> - const union decode_item *actions) >> + const union decode_item *table, bool thumb, >> + bool emulate, const union decode_item *actions) >> { >> struct decode_header *h = (struct decode_header *)table; >> struct decode_header *next; >> bool matched = false; >> >> - insn = prepare_emulated_insn(insn, asi, thumb); >> + if (emulate) >> + insn = prepare_emulated_insn(insn, asi, thumb); >> >> for (;; h = next) { >> enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK; >> @@ -402,7 +410,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> if (!matched && (insn & h->mask.bits) != h->value.bits) >> continue; >> >> - if (!decode_regs(&insn, regs)) >> + if (!decode_regs(&insn, regs, emulate)) >> return INSN_REJECTED; >> >> switch (type) { >> @@ -415,7 +423,8 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> >> case DECODE_TYPE_CUSTOM: { >> struct decode_custom *d = (struct decode_custom *)h; >> - return actions[d->decoder.bits].decoder(insn, asi, h); >> + return actions[d->decoder.bits].decoder(insn, >> + asi, h); > > No need to split the above line, you haven't changed it and it doesn't > exceed 80 characters anyway. Fixed. > [Rest of patch cut] > -dl
On Wed, 2014-01-15 at 14:31 -0500, David Long wrote: > On 12/20/13 09:58, Jon Medhurst (Tixy) wrote: > > On Sun, 2013-12-15 at 23:08 -0500, David Long wrote: [...] > >> { > >> #ifdef CONFIG_THUMB2_KERNEL > >> if (thumb) { > >> @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, > >> * non-zero value, the corresponding nibble in pinsn is validated and modified > >> * according to the type. > >> */ > >> -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) > >> +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify) > >> { > >> probes_opcode_t insn = *pinsn; > >> probes_opcode_t mask = 0xf; /* Start at least significant nibble */ > >> @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) > >> /* Replace value of nibble with new register number... */ > >> insn &= ~mask; > >> insn |= new_bits & mask; > >> + if (modify) { > >> + /* Replace value of nibble with new register number */ > >> + insn &= ~mask; > >> + insn |= new_bits & mask; > >> + } > > > > Huh? As is, the above addition doesn't do anything because insn has > > already been modified. I guess you played with the idea that you needed > > to avoid changing insn (you don't) and then didn't undo the experiment > > quite right. :-) > > > > The conditional modification of the instruction was part of Rabin's > original work for uprobes, but I messed up the merge from an earlier > working version of my patches. My intention was/is to delete the old > unconditional code. Sounds like maybe you disagree though. The intent > is to only modify the instruction in the kprobes case. 'insn' is the local variable containing the instruction value we're processing. It doesn't matter if we change that, we just need to avoid updating the instruction in memory, which the code in the next chunk already correctly checks for... > >> } > >> > >> - *pinsn = insn; > >> + if (modify) > >> + *pinsn = insn; > >> + > >> return true; > >> So only one of these 'if (modify)' checks is required for code correctness, and I suggest keeping the second one as it's more explicit and defensive.
On 01/16/14 04:18, Jon Medhurst (Tixy) wrote: > On Wed, 2014-01-15 at 14:31 -0500, David Long wrote: >> On 12/20/13 09:58, Jon Medhurst (Tixy) wrote: >>> On Sun, 2013-12-15 at 23:08 -0500, David Long wrote: > [...] >>>> { >>>> #ifdef CONFIG_THUMB2_KERNEL >>>> if (thumb) { >>>> @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >>>> * non-zero value, the corresponding nibble in pinsn is validated and modified >>>> * according to the type. >>>> */ >>>> -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) >>>> +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify) >>>> { >>>> probes_opcode_t insn = *pinsn; >>>> probes_opcode_t mask = 0xf; /* Start at least significant nibble */ >>>> @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) >>>> /* Replace value of nibble with new register number... */ >>>> insn &= ~mask; >>>> insn |= new_bits & mask; >>>> + if (modify) { >>>> + /* Replace value of nibble with new register number */ >>>> + insn &= ~mask; >>>> + insn |= new_bits & mask; >>>> + } >>> >>> Huh? As is, the above addition doesn't do anything because insn has >>> already been modified. I guess you played with the idea that you needed >>> to avoid changing insn (you don't) and then didn't undo the experiment >>> quite right. :-) >>> >> >> The conditional modification of the instruction was part of Rabin's >> original work for uprobes, but I messed up the merge from an earlier >> working version of my patches. My intention was/is to delete the old >> unconditional code. Sounds like maybe you disagree though. The intent >> is to only modify the instruction in the kprobes case. > > 'insn' is the local variable containing the instruction value we're > processing. It doesn't matter if we change that, we just need to avoid > updating the instruction in memory, which the code in the next chunk > already correctly checks for... > >>>> } >>>> >>>> - *pinsn = insn; >>>> + if (modify) >>>> + *pinsn = insn; >>>> + >>>> return true; >>>> > > So only one of these 'if (modify)' checks is required for code > correctness, and I suggest keeping the second one as it's more explicit > and defensive. > > OK, I see your point. I shall simplify the code as you have suggested. -dl
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c index 0d9d49b..04690f9 100644 --- a/arch/arm/kernel/kprobes.c +++ b/arch/arm/kernel/kprobes.c @@ -87,7 +87,8 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p) p->opcode = insn; p->ainsn.insn = tmp_insn; - switch ((*decode_insn)(insn, &p->ainsn, actions)) { + switch ((*decode_insn)(insn, &p->ainsn, + true, actions)) { case INSN_REJECTED: /* not supported */ return -EINVAL; diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/kernel/kprobes.h index e1305a8..580d82c 100644 --- a/arch/arm/kernel/kprobes.h +++ b/arch/arm/kernel/kprobes.h @@ -35,6 +35,7 @@ kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi, typedef enum probes_insn (kprobe_decode_insn_t)(kprobe_opcode_t, struct arch_specific_insn *, + bool, const union decode_item *); #ifdef CONFIG_THUMB2_KERNEL diff --git a/arch/arm/kernel/probes-arm.c b/arch/arm/kernel/probes-arm.c index a44c3ad..01e99f7 100644 --- a/arch/arm/kernel/probes-arm.c +++ b/arch/arm/kernel/probes-arm.c @@ -723,10 +723,10 @@ static void __kprobes arm_singlestep(probes_opcode_t insn, */ enum probes_insn __kprobes arm_probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *actions) + bool emulate, const union decode_item *actions) { asi->insn_singlestep = arm_singlestep; asi->insn_check_cc = probes_condition_checks[insn>>28]; return probes_decode_insn(insn, asi, probes_decode_arm_table, false, - actions); + emulate, actions); } diff --git a/arch/arm/kernel/probes-arm.h b/arch/arm/kernel/probes-arm.h index 7f81199..904033c 100644 --- a/arch/arm/kernel/probes-arm.h +++ b/arch/arm/kernel/probes-arm.h @@ -66,7 +66,7 @@ void __kprobes simulate_mov_ipsp(probes_opcode_t opcode, extern const union decode_item probes_decode_arm_table[]; enum probes_insn arm_probes_decode_insn(probes_opcode_t, - struct arch_specific_insn *, + struct arch_specific_insn *, bool emulate, const union decode_item *actions); #endif diff --git a/arch/arm/kernel/probes-thumb.c b/arch/arm/kernel/probes-thumb.c index 3f250e9..2ad0880 100644 --- a/arch/arm/kernel/probes-thumb.c +++ b/arch/arm/kernel/probes-thumb.c @@ -861,20 +861,20 @@ static void __kprobes thumb32_singlestep(probes_opcode_t opcode, enum probes_insn __kprobes thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *actions) + bool emulate, const union decode_item *actions) { asi->insn_singlestep = thumb16_singlestep; asi->insn_check_cc = thumb_check_cc; return probes_decode_insn(insn, asi, probes_decode_thumb16_table, true, - actions); + emulate, actions); } enum probes_insn __kprobes thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *actions) + bool emulate, const union decode_item *actions) { asi->insn_singlestep = thumb32_singlestep; asi->insn_check_cc = thumb_check_cc; return probes_decode_insn(insn, asi, probes_decode_thumb32_table, true, - actions); + emulate, actions); } diff --git a/arch/arm/kernel/probes-thumb.h b/arch/arm/kernel/probes-thumb.h index 556f067..78be667 100644 --- a/arch/arm/kernel/probes-thumb.h +++ b/arch/arm/kernel/probes-thumb.h @@ -87,9 +87,9 @@ extern const union decode_item probes_decode_thumb16_table[]; enum probes_insn __kprobes thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *actions); + bool emulate, const union decode_item *actions); enum probes_insn __kprobes thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *actions); + bool emulate, const union decode_item *actions); #endif diff --git a/arch/arm/kernel/probes.c b/arch/arm/kernel/probes.c index a0a4220..e691e12 100644 --- a/arch/arm/kernel/probes.c +++ b/arch/arm/kernel/probes.c @@ -193,7 +193,7 @@ void __kprobes probes_emulate_none(probes_opcode_t opcode, */ static probes_opcode_t __kprobes prepare_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - bool thumb) + bool thumb) { #ifdef CONFIG_THUMB2_KERNEL if (thumb) { @@ -218,7 +218,7 @@ prepare_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, */ static void __kprobes set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - bool thumb) + bool thumb) { #ifdef CONFIG_THUMB2_KERNEL if (thumb) { @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi, * non-zero value, the corresponding nibble in pinsn is validated and modified * according to the type. */ -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify) { probes_opcode_t insn = *pinsn; probes_opcode_t mask = 0xf; /* Start at least significant nibble */ @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs) /* Replace value of nibble with new register number... */ insn &= ~mask; insn |= new_bits & mask; + if (modify) { + /* Replace value of nibble with new register number */ + insn &= ~mask; + insn |= new_bits & mask; + } } - *pinsn = insn; + if (modify) + *pinsn = insn; + return true; reject: @@ -380,14 +387,15 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = { */ int __kprobes probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *table, bool thumb, - const union decode_item *actions) + const union decode_item *table, bool thumb, + bool emulate, const union decode_item *actions) { struct decode_header *h = (struct decode_header *)table; struct decode_header *next; bool matched = false; - insn = prepare_emulated_insn(insn, asi, thumb); + if (emulate) + insn = prepare_emulated_insn(insn, asi, thumb); for (;; h = next) { enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK; @@ -402,7 +410,7 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, if (!matched && (insn & h->mask.bits) != h->value.bits) continue; - if (!decode_regs(&insn, regs)) + if (!decode_regs(&insn, regs, emulate)) return INSN_REJECTED; switch (type) { @@ -415,7 +423,8 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, case DECODE_TYPE_CUSTOM: { struct decode_custom *d = (struct decode_custom *)h; - return actions[d->decoder.bits].decoder(insn, asi, h); + return actions[d->decoder.bits].decoder(insn, + asi, h); } case DECODE_TYPE_SIMULATE: { @@ -426,6 +435,11 @@ probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, case DECODE_TYPE_EMULATE: { struct decode_emulate *d = (struct decode_emulate *)h; + + if (!emulate) + return actions[d->handler.bits].decoder(insn, + asi, h); + asi->insn_handler = actions[d->handler.bits].handler; set_emulated_insn(insn, asi, thumb); return INSN_GOOD; diff --git a/arch/arm/kernel/probes.h b/arch/arm/kernel/probes.h index 9591c2c..233ab7b 100644 --- a/arch/arm/kernel/probes.h +++ b/arch/arm/kernel/probes.h @@ -389,7 +389,7 @@ probes_insn_handler_t probes_emulate_none; int __kprobes probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, - const union decode_item *table, bool thumb, + const union decode_item *table, bool thumb, bool emulate, const union decode_item *actions); #endif