uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

Purely cosmetic, no changes in .o,

1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to ->dflt.

2. Add the comment into default_post_xol_op() to explain "regs->sp +=".

3. Remove the stale part of the comment in arch_uprobe_analyze_insn().

Suggested-by Jim Keniston <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
 2 files changed, 19 insertions(+), 20 deletions(-)

arch/x86/include/asm/uprobes.h +1 -1

index 7be3c07..b3d9442 100644
@@ -52,7 +52,7 @@ struct arch_uprobe {
 		struct {
 			u8	fixups;
 			u8	ilen;
-		} 			def;
+		} 			dflt;
 	};
 };
 

arch/x86/kernel/uprobes.c +18 -19

index fcf6279..33e239f 100644
@@ -254,7 +254,7 @@ static inline bool is_64bit_mm(struct mm_struct *mm)
  * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
  * its memory operand indirectly through a scratch register.  Set
- * def->fixups accordingly. (The contents of the scratch register
+ * dflt->fixups accordingly. (The contents of the scratch register
  * will be saved before we single-step the modified instruction,
  * and restored afterward).
  *

@@ -372,14 +372,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 	 */
 	if (reg != 6 && reg2 != 6) {
 		reg2 = 6;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_SI;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_SI;
 	} else if (reg != 7 && reg2 != 7) {
 		reg2 = 7;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_DI;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_DI;
 		/* TODO (paranoia): force maskmovq to not use di */
 	} else {
 		reg2 = 3;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_BX;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_BX;
 	}
 	/*
 	 * Point cursor at the modrm byte.  The next 4 bytes are the

@@ -398,9 +398,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 static inline unsigned long *
 scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_SI)
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_SI)
 		return &regs->si;
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_DI)
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_DI)
 		return &regs->di;
 	return &regs->bx;
 }

@@ -411,18 +411,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
  */
 static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
 		struct uprobe_task *utask = current->utask;
 		unsigned long *sr = scratch_reg(auprobe, regs);
 
 		utask->autask.saved_scratch_register = *sr;
-		*sr = utask->vaddr + auprobe->def.ilen;
+		*sr = utask->vaddr + auprobe->dflt.ilen;
 	}
 }
 
 static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
 		struct uprobe_task *utask = current->utask;
 		unsigned long *sr = scratch_reg(auprobe, regs);
 

@@ -499,16 +499,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
 	struct uprobe_task *utask = current->utask;
 
 	riprel_post_xol(auprobe, regs);
-	if (auprobe->def.fixups & UPROBE_FIX_IP) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_IP) {
 		long correction = utask->vaddr - utask->xol_vaddr;
 		regs->ip += correction;
-	} else if (auprobe->def.fixups & UPROBE_FIX_CALL) {
-		regs->sp += sizeof_long();
-		if (push_ret_address(regs, utask->vaddr + auprobe->def.ilen))
+	} else if (auprobe->dflt.fixups & UPROBE_FIX_CALL) {
+		regs->sp += sizeof_long(); /* Pop incorrect return address */
+		if (push_ret_address(regs, utask->vaddr + auprobe->dflt.ilen))
 			return -ERESTART;
 	}
 	/* popf; tell the caller to not touch TF */
-	if (auprobe->def.fixups & UPROBE_FIX_SETF)
+	if (auprobe->dflt.fixups & UPROBE_FIX_SETF)
 		utask->autask.saved_tf = true;
 
 	return 0;

@@ -711,12 +711,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 
 	/*
 	 * Figure out which fixups default_post_xol_op() will need to perform,
-	 * and annotate def->fixups accordingly. To start with, ->fixups is
-	 * either zero or it reflects rip-related fixups.
+	 * and annotate dflt->fixups accordingly.
 	 */
 	switch (OPCODE1(&insn)) {
 	case 0x9d:		/* popf */
-		auprobe->def.fixups |= UPROBE_FIX_SETF;
+		auprobe->dflt.fixups |= UPROBE_FIX_SETF;
 		break;
 	case 0xc3:		/* ret or lret -- ip is correct */
 	case 0xcb:

@@ -742,8 +741,8 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		riprel_analyze(auprobe, &insn);
 	}
 
-	auprobe->def.ilen = insn.length;
-	auprobe->def.fixups |= fix_ip_or_call;
+	auprobe->dflt.ilen = insn.length;
+	auprobe->dflt.fixups |= fix_ip_or_call;
 
 	auprobe->ops = &default_xol_ops;
 	return 0;

Purely cosmetic, no changes in .o,

1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to ->dflt.

2. Add the comment into default_post_xol_op() to explain "regs->sp +=".

3. Remove the stale part of the comment in arch_uprobe_analyze_insn().

Suggested-by Jim Keniston <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---

Purely cosmetic, no changes in .o,

1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to ->dflt.

2. Add the comment into default_post_xol_op() to explain "regs->sp +=".

3. Remove the stale part of the comment in arch_uprobe_analyze_insn().

Suggested-by Jim Keniston <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
 2 files changed, 19 insertions(+), 20 deletions(-)

arch/x86/include/asm/uprobes.h +1 -1

index 7be3c07..b3d9442 100644
@@ -52,7 +52,7 @@ struct arch_uprobe {
 		struct {
 			u8	fixups;
 			u8	ilen;
-		} 			def;
+		} 			dflt;
 	};
 };
 

arch/x86/kernel/uprobes.c +18 -19

index fcf6279..33e239f 100644
@@ -254,7 +254,7 @@ static inline bool is_64bit_mm(struct mm_struct *mm)
  * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
  * its memory operand indirectly through a scratch register.  Set
- * def->fixups accordingly. (The contents of the scratch register
+ * dflt->fixups accordingly. (The contents of the scratch register
  * will be saved before we single-step the modified instruction,
  * and restored afterward).
  *

@@ -372,14 +372,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 	 */
 	if (reg != 6 && reg2 != 6) {
 		reg2 = 6;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_SI;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_SI;
 	} else if (reg != 7 && reg2 != 7) {
 		reg2 = 7;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_DI;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_DI;
 		/* TODO (paranoia): force maskmovq to not use di */
 	} else {
 		reg2 = 3;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_BX;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_BX;
 	}
 	/*
 	 * Point cursor at the modrm byte.  The next 4 bytes are the

@@ -398,9 +398,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 static inline unsigned long *
 scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_SI)
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_SI)
 		return &regs->si;
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_DI)
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_DI)
 		return &regs->di;
 	return &regs->bx;
 }

@@ -411,18 +411,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
  */
 static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
 		struct uprobe_task *utask = current->utask;
 		unsigned long *sr = scratch_reg(auprobe, regs);
 
 		utask->autask.saved_scratch_register = *sr;
-		*sr = utask->vaddr + auprobe->def.ilen;
+		*sr = utask->vaddr + auprobe->dflt.ilen;
 	}
 }
 
 static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
 		struct uprobe_task *utask = current->utask;
 		unsigned long *sr = scratch_reg(auprobe, regs);
 

@@ -499,16 +499,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
 	struct uprobe_task *utask = current->utask;
 
 	riprel_post_xol(auprobe, regs);
-	if (auprobe->def.fixups & UPROBE_FIX_IP) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_IP) {
 		long correction = utask->vaddr - utask->xol_vaddr;
 		regs->ip += correction;
-	} else if (auprobe->def.fixups & UPROBE_FIX_CALL) {
-		regs->sp += sizeof_long();
-		if (push_ret_address(regs, utask->vaddr + auprobe->def.ilen))
+	} else if (auprobe->dflt.fixups & UPROBE_FIX_CALL) {
+		regs->sp += sizeof_long(); /* Pop incorrect return address */
+		if (push_ret_address(regs, utask->vaddr + auprobe->dflt.ilen))
 			return -ERESTART;
 	}
 	/* popf; tell the caller to not touch TF */
-	if (auprobe->def.fixups & UPROBE_FIX_SETF)
+	if (auprobe->dflt.fixups & UPROBE_FIX_SETF)
 		utask->autask.saved_tf = true;
 
 	return 0;

@@ -711,12 +711,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 
 	/*
 	 * Figure out which fixups default_post_xol_op() will need to perform,
-	 * and annotate def->fixups accordingly. To start with, ->fixups is
-	 * either zero or it reflects rip-related fixups.
+	 * and annotate dflt->fixups accordingly.
 	 */
 	switch (OPCODE1(&insn)) {
 	case 0x9d:		/* popf */
-		auprobe->def.fixups |= UPROBE_FIX_SETF;
+		auprobe->dflt.fixups |= UPROBE_FIX_SETF;
 		break;
 	case 0xc3:		/* ret or lret -- ip is correct */
 	case 0xcb:

@@ -742,8 +741,8 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		riprel_analyze(auprobe, &insn);
 	}
 
-	auprobe->def.ilen = insn.length;
-	auprobe->def.fixups |= fix_ip_or_call;
+	auprobe->dflt.ilen = insn.length;
+	auprobe->dflt.fixups |= fix_ip_or_call;
 
 	auprobe->ops = &default_xol_ops;
 	return 0;

Purely cosmetic, no changes in .o,

1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to ->dflt.

2. Add the comment into default_post_xol_op() to explain "regs->sp +=".

3. Remove the stale part of the comment in arch_uprobe_analyze_insn().

Suggested-by Jim Keniston <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
 2 files changed, 19 insertions(+), 20 deletions(-)

arch/x86/include/asm/uprobes.h +0 -0

index 7be3c07..b3d9442 100644

arch/x86/include/asm/uprobes.h struct arch_uprobe { · line 55

Quoted 8 lines · Oleg Nesterov

arch/x86/include/asm/uprobes.h +1 -1

@@ -52,7 +52,7 @@ struct arch_uprobe {
 		struct {
 			u8	fixups;
 			u8	ilen;
-		} 			def;
+		} 			dflt;

Pls lts nt use slly abbrvtns, ok?

How about arch_uprobe->default?

Quoted 13 lines · Ingo Molnar

arch/x86/include/asm/uprobes.h struct arch_uprobe { · line 55

Quoted 8 lines · Oleg Nesterov

arch/x86/include/asm/uprobes.h +1 -1

@@ -52,7 +52,7 @@ struct arch_uprobe {
 		struct {
 			u8	fixups;
 			u8	ilen;
-		} 			def;
+		} 			dflt;

Pls lts nt use slly abbrvtns, ok?

OK. As I said in the previous dicussion, I agree with any naming.

Quoted 1 line

How about arch_uprobe->default?

And this is how it was named when I wrote this code. Unfortunately gcc dislikes this name ;) So I renamed it to ->def. Then I was asked to rename it and I agree, ->def doesn't look good.

Could you suggest something better?

Quoted 5 lines · Oleg Nesterov

And this is how it was named when I wrote this code. Unfortunately gcc dislikes this name ;) So I renamed it to ->def. Then I was asked to rename it and I agree, ->def doesn't look good.

Could you suggest something better?

So exactly what do those fields do? If it's scratch register handling, would it be logical to name it arch_uprobe->scratch, or so?

Quoted 11 lines · Ingo Molnar

Quoted 5 lines · Oleg Nesterov

And this is how it was named when I wrote this code. Unfortunately gcc dislikes this name ;) So I renamed it to ->def. Then I was asked to rename it and I agree, ->def doesn't look good.

Could you suggest something better?

So exactly what do those fields do? If it's scratch register handling, would it be logical to name it arch_uprobe->scratch, or so?

Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.

arch_uprobe->def contains the arguments for default_xol_ops methods, currently this handles everything except relative jmp/call insns.

So perhaps ->dflt is not that ugly in this case? I simply do not see anything better. But again, I agree with any name in advance.

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Ingo Molnar

So exactly what do those fields do? If it's scratch register handling, would it be logical to name it arch_uprobe->scratch, or so?

Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.

Quoted 5 lines

arch_uprobe->def contains the arguments for default_xol_ops methods, currently this handles everything except relative jmp/call insns.

So perhaps ->dflt is not that ugly in this case? I simply do not see anything better. But again, I agree with any name in advance.

Hmm, how about ->defparam ? :)

Quoted 12 lines · Masami Hiramatsu

Quoted 10 lines · Oleg Nesterov

Quoted 3 lines · Ingo Molnar

So exactly what do those fields do? If it's scratch register handling, would it be logical to name it arch_uprobe->scratch, or so?

Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.

arch_uprobe->def contains the arguments for default_xol_ops methods, currently this handles everything except relative jmp/call insns.

Quoted 4 lines

Quoted 2 lines

So perhaps ->dflt is not that ugly in this case? I simply do not see anything better. But again, I agree with any name in advance.

Hmm, how about ->defparam ? :)

Fine with me ;)

Ingo, will you agree with s/def/defparam/ ?

Quoted 2 lines · Oleg Nesterov

Quoted 1 line · Masami Hiramatsu

Quoted 5 lines

Quoted 1 line

Hmm, how about ->defparam ? :)

Fine with me ;)

Ingo, will you agree with s/def/defparam/ ?

Certainly! :)

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Oleg Nesterov

Quoted 1 line · Masami Hiramatsu

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Hmm, how about ->defparam ? :)

Fine with me ;)

Ingo, will you agree with s/def/defparam/ ?

Certainly! :)

Great ;) I updated this patch in uprobes/core, please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core

based on tip:perf/uprobes

Oleg Nesterov (3):

      uprobes: Shift ->readpage check from __copy_insn() to uprobe_register()
      uprobes: Teach copy_insn() to support tmpfs
      uprobes/x86: Rename arch_uprobe->def to ->defparam, minor comment updates

 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
 kernel/events/uprobes.c        |   17 +++++++++++------
 3 files changed, 30 insertions(+), 26 deletions(-)