Re: [PATCH v4 05/16] ARM: use a function table for determininginstruction interpreter actions

[Jon Medhurst (Tixy)]

On Sun, 2013-12-15 at 23:08 -0500, David Long wrote:
> From: "David A. Long" <dave.long@xxxxxxxxxx>
> 
> 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 <dave.long@xxxxxxxxxx>
> ---

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);


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.


> +	[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.

-- 
Tixy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/