On 12/20/13 07:45, Jon Medhurst (Tixy) wrote:
> On Sun, 2013-12-15 at 23:08 -0500, David Long wrote:
>> From: "David A. Long" <[email protected]>
>>
>> Make the instruction interpreter call back to semantic action functions
>> through a function pointer array provided by the invoker. The interpreter
>> decodes the instructions into groups and uses the group number to index
>> into the supplied array. kprobes and uprobes code will each supply their
>> own array of functions.
>>
>> Signed-off-by: David A. Long <[email protected]>
>> ---
>
> Because I've been very slow in reviewing these I've only just noticed
> that some of the the comments I made on version one of this patch didn't
> get a response. I've copied them again below (slightly edited) and
> heavily trimmed the patch...
>
>> arch/arm/kernel/kprobes-arm.c | 41 +++++++++++
>> arch/arm/kernel/kprobes-common.c | 3 +-
>> arch/arm/kernel/kprobes-thumb.c | 92 ++++++++++++++++++------
>> arch/arm/kernel/kprobes.c | 10 ++-
>> arch/arm/kernel/kprobes.h | 14 ++--
>> arch/arm/kernel/probes-arm.c | 114 +++++++++++++++---------------
>> arch/arm/kernel/probes-arm.h | 37 ++++++++++
>> arch/arm/kernel/probes-thumb.c | 149 +++++++++++++++++++--------------------
>> arch/arm/kernel/probes-thumb.h | 14 ++--
>> arch/arm/kernel/probes.c | 13 ++--
>> arch/arm/kernel/probes.h | 15 ++--
>> 11 files changed, 325 insertions(+), 177 deletions(-)
>>
>> diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
>> index a359475..ee329ff 100644
>> --- a/arch/arm/kernel/kprobes-arm.c
>> +++ b/arch/arm/kernel/kprobes-arm.c
>> @@ -299,3 +299,44 @@ emulate_rdlo12rdhi16rn0rm8_rwflags_nopc(struct kprobe *p, struct pt_regs *regs)
>> regs->uregs[rdhi] = rdhiv;
>> regs->ARM_cpsr = (regs->ARM_cpsr & ~APSR_MASK) | (cpsr & APSR_MASK);
>> }
>> +
>> +const union decode_item kprobes_arm_actions[] = {
>
> I think it's best if we don't reuse the decode_item type here, this is a
> different sort of table so probably best to have it's own union. Also,
> if we do that, then decode_item can be simplified as it won't need to
> have function pointers in it, i.e. we could end up with...
>
> union decode_action {
> kprobe_insn_handler_t *handler;
> kprobe_custom_decode_t *decoder;
> };
>
> union decode_item {
> u32 bits;
> const union decode_item *table;
> };
>
> typedef enum kprobe_insn (kprobe_custom_decode_t)(kprobe_opcode_t,
> struct arch_specific_insn *,
> union decode_action *actions);
>
>
I've added the following:
typedef enum kprobe_insn (probes_custom_decode_t)(kprobe_opcode_t,
struct arch_specific_insn *,
struct decode_header *);
union decode_action {
kprobe_insn_handler_t *handler;
probes_custom_decode_t *decoder;
};
Note the third argument actually passed into the decoder functions is
the decode table entry. decode_action is only used to select a
decode/emulate/simullate function.
> A second point, I think it would be a good idea to make sure these
> action arrays are the size we expect by adding an entry at the end of
> the relevant enumeration and using that to set the size of the arrays.
> E.g. for this one
>
> enum probes_arm_action {
> ...
> ...
> NUM_PROBES_ARM_ACTIONS
> };
>
> and then use it like:
>
> const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
>
> That way, we at least make any uninitialised entries are null (I
> assume?) which is safer than accidentally indexing beyond the array.
>
>
Done.
>> + [PROBES_EMULATE_NONE] = {.handler = kprobe_emulate_none},
>> + [PROBES_SIMULATE_NOP] = {.handler = kprobe_simulate_nop},
>> + [PROBES_PRELOAD_IMM] = {.handler = kprobe_simulate_nop},
>
> [...]
>
>
>> diff --git a/arch/arm/kernel/probes.h b/arch/arm/kernel/probes.h
>> index d14d224..2238972 100644
>> --- a/arch/arm/kernel/probes.h
>> +++ b/arch/arm/kernel/probes.h
>> @@ -131,7 +131,8 @@ void __kprobes kprobe_simulate_nop(struct kprobe *p, struct pt_regs *regs);
>> void __kprobes kprobe_emulate_none(struct kprobe *p, struct pt_regs *regs);
>>
>> enum kprobe_insn __kprobes
>> -kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi);
>> +kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
>> + struct decode_header *h);
>>
>> /*
>> * Test if load/store instructions writeback the address register.
>> @@ -334,7 +335,7 @@ struct decode_custom {
>>
>> #define DECODE_CUSTOM(_mask, _value, _decoder) \
>> DECODE_HEADER(DECODE_TYPE_CUSTOM, _mask, _value, 0), \
>> - {.decoder = (_decoder)}
>> + {.bits = (_decoder)}
>>
>
> This third and final comment is probably just bike shedding...
>
> 'bits' looks a bit funny here. I've been trying to think of a way of
> making it nicer but it's difficult. The actual value is one of three
> different enums, so if we were to add another members to decode_item it
> would just have to be "int action", at least that would read nicer in
> these macros and where it gets read out in probes_decode_insn.
>
I agree. I've added an "int action" to the decode_item union, and use
it instead of "bits" for the action array index.
I've also updated the description of how this all works, in the comments.
-dl