On 12/20/13 09:58, Jon Medhurst (Tixy) wrote:

On Sun, 2013-12-15 at 23:08 -0500, David Long wrote:

quoted

From: "David A. Long" <redacted>

Add an emulate flag into the instruction interpreter, primarily for uprobes
support.

Signed-off-by: David A. Long <redacted>
---
  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.

quoted

  	case INSN_REJECTED:	/* not supported */
  		return -EINVAL;

[...]

quoted

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

quoted

  {
  #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.

quoted

  {
  #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.

quoted

  	}

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