Re: [PATCH v2 11/13] ARM: Finish renaming ARM kprobes APIs forsharing with uprobes

[Jon Medhurst (Tixy)]

On Tue, 2013-10-15 at 17:04 -0400, David Long wrote:
> From: "David A. Long" <dave.long@xxxxxxxxxx>
> 
> Use the prefix "probes_" for APIs and definitions to be shared between
> kprobes and uprobes. Pass additional information into the lower-level decoding
> functions to avoid having to duplicate architecture-specfific data structures.
> Make susbsystem-specific APIs static (non-global) again.  Make a couple of utility
> functions (probes_simulate_nop and probes_emulate_none) sharable and in a common
> file. Keep the current "action" tables specific to kprobes, as this is where
> uprobes functionality will be connected up to the instruction interpreter.

That's a rather long list of things this patch is reorganising and makes
it very difficult to review. For those class of changes which are
independent of each other, can you do them as separate patches?

Particularly, the change from using the function arguments

    struct kprobe *p'

to
    probes_opcode_t opcode, probes_opcode_t *addr, struct arch_specific_insn *asi

should be in it's own separate patch as that is spread out across multiple files
and functions.

I have a few general comments on this patch...

1. I don't think we actually need to pass 'probes_opcode_t *addr' to the
simulation functions, this will be the same as regs->ARM_pc (except that
for thumb code 'addr' has bit0 set, which we don't actually care about).
So drop the new 'addr' argument, and we can also delete the definition of
thumb_probe_pc() and replace its use with 'regs->ARM_pc + 4' 

2. Now we pass 'probes_opcode_t opcode' to simulation functions a lot of
them end up doing the redundant...

   kprobe_opcode_t insn = opcode

I would suggest deleting all of these assignments and either rename all
use of 'insn' to 'opcode' or just changing the function argument name to
'insn'.

3. I've a comment about the change to probes_decode_insn, I've snipped
the rest of the patch...

[...]
>  int __kprobes
> -kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
> +probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi,
>  				const union decode_item *table, bool thumb,
> -				const union decode_item *actions)
> +				bool usermode, const union decode_item *actions)
>  {

The new argument 'usermode' is named for the use you are using it for,
i.e. uprobes, I think the code is more intuitive if it was called
something like 'no_emulate', because what the argument does is force all
emulate operations to become 'custom'.

Perhaps to avoid double negatives like !no_emulate, the logic of the
argument could be inverted and it called 'use_emulation' or something.

> -	const struct decode_header *h = (struct decode_header *)table;
> -	const struct decode_header *next;
> +	struct decode_header *h = (struct decode_header *)table;
> +	struct decode_header *next;
>  	bool matched = false;
>  
> -	insn = prepare_emulated_insn(insn, asi, thumb);
> +	if (!usermode)
> +		insn = prepare_emulated_insn(insn, asi, thumb);
>  
>  	for (;; h = next) {
>  		enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK;
> @@ -401,7 +412,7 @@ kprobe_decode_insn(kprobe_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, !usermode))
>  			return INSN_REJECTED;
>  
>  		switch (type) {
> @@ -414,7 +425,8 @@ kprobe_decode_insn(kprobe_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: {
> @@ -425,6 +437,11 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
>  
>  		case DECODE_TYPE_EMULATE: {
>  			struct decode_emulate *d = (struct decode_emulate *)h;
> +
> +			if (usermode)
> +				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;

[...]

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