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

[David Long]

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

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

--
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/