diff mbox series

[v2,1/3] x86/kprobes: Refactor can_{probe,boost} return type to bool

Message ID 20240204031300.830475-2-jinghao7@illinois.edu
State New
Headers
Series x86/kprobes: add exception opcode detector and boost more opcodes |

Commit Message

Jinghao Jia Feb. 4, 2024, 3:12 a.m. UTC 
  Both can_probe and can_boost have int return type but are using int as
boolean in their context.

Refactor both functions to make them actually return boolean.

Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
---
 arch/x86/kernel/kprobes/common.h |  2 +-
 arch/x86/kernel/kprobes/core.c   | 33 +++++++++++++++-----------------
 2 files changed, 16 insertions(+), 19 deletions(-)

Comments

Masami Hiramatsu (Google) Feb. 4, 2024, 12:10 p.m. UTC | #1 
On Sat,  3 Feb 2024 21:12:58 -0600
Jinghao Jia <jinghao7@illinois.edu> wrote:

> Both can_probe and can_boost have int return type but are using int as
> boolean in their context.
> 
> Refactor both functions to make them actually return boolean.
> 

This and next one looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Let me pick those.

> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
> ---
>  arch/x86/kernel/kprobes/common.h |  2 +-
>  arch/x86/kernel/kprobes/core.c   | 33 +++++++++++++++-----------------
>  2 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/common.h b/arch/x86/kernel/kprobes/common.h
> index c993521d4933..e772276f5aa9 100644
> --- a/arch/x86/kernel/kprobes/common.h
> +++ b/arch/x86/kernel/kprobes/common.h
> @@ -78,7 +78,7 @@
>  #endif
>  
>  /* Ensure if the instruction can be boostable */
> -extern int can_boost(struct insn *insn, void *orig_addr);
> +extern bool can_boost(struct insn *insn, void *orig_addr);
>  /* Recover instruction if given address is probed */
>  extern unsigned long recover_probed_instruction(kprobe_opcode_t *buf,
>  					 unsigned long addr);
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index e8babebad7b8..644d416441fb 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -137,14 +137,14 @@ NOKPROBE_SYMBOL(synthesize_relcall);
>   * Returns non-zero if INSN is boostable.
>   * RIP relative instructions are adjusted at copying time in 64 bits mode
>   */
> -int can_boost(struct insn *insn, void *addr)
> +bool can_boost(struct insn *insn, void *addr)
>  {
>  	kprobe_opcode_t opcode;
>  	insn_byte_t prefix;
>  	int i;
>  
>  	if (search_exception_tables((unsigned long)addr))
> -		return 0;	/* Page fault may occur on this address. */
> +		return false;	/* Page fault may occur on this address. */
>  
>  	/* 2nd-byte opcode */
>  	if (insn->opcode.nbytes == 2)
> @@ -152,7 +152,7 @@ int can_boost(struct insn *insn, void *addr)
>  				(unsigned long *)twobyte_is_boostable);
>  
>  	if (insn->opcode.nbytes != 1)
> -		return 0;
> +		return false;
>  
>  	for_each_insn_prefix(insn, i, prefix) {
>  		insn_attr_t attr;
> @@ -160,7 +160,7 @@ int can_boost(struct insn *insn, void *addr)
>  		attr = inat_get_opcode_attribute(prefix);
>  		/* Can't boost Address-size override prefix and CS override prefix */
>  		if (prefix == 0x2e || inat_is_address_size_prefix(attr))
> -			return 0;
> +			return false;
>  	}
>  
>  	opcode = insn->opcode.bytes[0];
> @@ -181,12 +181,12 @@ int can_boost(struct insn *insn, void *addr)
>  	case 0xf6 ... 0xf7:	/* Grp3 */
>  	case 0xfe:		/* Grp4 */
>  		/* ... are not boostable */
> -		return 0;
> +		return false;
>  	case 0xff:		/* Grp5 */
>  		/* Only indirect jmp is boostable */
>  		return X86_MODRM_REG(insn->modrm.bytes[0]) == 4;
>  	default:
> -		return 1;
> +		return true;
>  	}
>  }
>  
> @@ -253,20 +253,18 @@ unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
>  }
>  
>  /* Check if paddr is at an instruction boundary */
> -static int can_probe(unsigned long paddr)
> +static bool can_probe(unsigned long paddr)
>  {
>  	unsigned long addr, __addr, offset = 0;
>  	struct insn insn;
>  	kprobe_opcode_t buf[MAX_INSN_SIZE];
>  
>  	if (!kallsyms_lookup_size_offset(paddr, NULL, &offset))
> -		return 0;
> +		return false;
>  
>  	/* Decode instructions */
>  	addr = paddr - offset;
>  	while (addr < paddr) {
> -		int ret;
> -
>  		/*
>  		 * Check if the instruction has been modified by another
>  		 * kprobe, in which case we replace the breakpoint by the
> @@ -277,11 +275,10 @@ static int can_probe(unsigned long paddr)
>  		 */
>  		__addr = recover_probed_instruction(buf, addr);
>  		if (!__addr)
> -			return 0;
> +			return false;
>  
> -		ret = insn_decode_kernel(&insn, (void *)__addr);
> -		if (ret < 0)
> -			return 0;
> +		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> +			return false;
>  
>  #ifdef CONFIG_KGDB
>  		/*
> @@ -290,7 +287,7 @@ static int can_probe(unsigned long paddr)
>  		 */
>  		if (insn.opcode.bytes[0] == INT3_INSN_OPCODE &&
>  		    kgdb_has_hit_break(addr))
> -			return 0;
> +			return false;
>  #endif
>  		addr += insn.length;
>  	}
> @@ -310,10 +307,10 @@ static int can_probe(unsigned long paddr)
>  		 */
>  		__addr = recover_probed_instruction(buf, addr);
>  		if (!__addr)
> -			return 0;
> +			return false;
>  
>  		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> -			return 0;
> +			return false;
>  
>  		if (insn.opcode.value == 0xBA)
>  			offset = 12;
> @@ -324,7 +321,7 @@ static int can_probe(unsigned long paddr)
>  
>  		/* This movl/addl is used for decoding CFI. */
>  		if (is_cfi_trap(addr + offset))
> -			return 0;
> +			return false;
>  	}
>  
>  out:
> -- 
> 2.43.0
>
diff mbox series

Patch

diff --git a/arch/x86/kernel/kprobes/common.h b/arch/x86/kernel/kprobes/common.h
index c993521d4933..e772276f5aa9 100644
--- a/arch/x86/kernel/kprobes/common.h
+++ b/arch/x86/kernel/kprobes/common.h
@@ -78,7 +78,7 @@ 
 #endif
 
 /* Ensure if the instruction can be boostable */
-extern int can_boost(struct insn *insn, void *orig_addr);
+extern bool can_boost(struct insn *insn, void *orig_addr);
 /* Recover instruction if given address is probed */
 extern unsigned long recover_probed_instruction(kprobe_opcode_t *buf,
 					 unsigned long addr);
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index e8babebad7b8..644d416441fb 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -137,14 +137,14 @@  NOKPROBE_SYMBOL(synthesize_relcall);
  * Returns non-zero if INSN is boostable.
  * RIP relative instructions are adjusted at copying time in 64 bits mode
  */
-int can_boost(struct insn *insn, void *addr)
+bool can_boost(struct insn *insn, void *addr)
 {
 	kprobe_opcode_t opcode;
 	insn_byte_t prefix;
 	int i;
 
 	if (search_exception_tables((unsigned long)addr))
-		return 0;	/* Page fault may occur on this address. */
+		return false;	/* Page fault may occur on this address. */
 
 	/* 2nd-byte opcode */
 	if (insn->opcode.nbytes == 2)
@@ -152,7 +152,7 @@  int can_boost(struct insn *insn, void *addr)
 				(unsigned long *)twobyte_is_boostable);
 
 	if (insn->opcode.nbytes != 1)
-		return 0;
+		return false;
 
 	for_each_insn_prefix(insn, i, prefix) {
 		insn_attr_t attr;
@@ -160,7 +160,7 @@  int can_boost(struct insn *insn, void *addr)
 		attr = inat_get_opcode_attribute(prefix);
 		/* Can't boost Address-size override prefix and CS override prefix */
 		if (prefix == 0x2e || inat_is_address_size_prefix(attr))
-			return 0;
+			return false;
 	}
 
 	opcode = insn->opcode.bytes[0];
@@ -181,12 +181,12 @@  int can_boost(struct insn *insn, void *addr)
 	case 0xf6 ... 0xf7:	/* Grp3 */
 	case 0xfe:		/* Grp4 */
 		/* ... are not boostable */
-		return 0;
+		return false;
 	case 0xff:		/* Grp5 */
 		/* Only indirect jmp is boostable */
 		return X86_MODRM_REG(insn->modrm.bytes[0]) == 4;
 	default:
-		return 1;
+		return true;
 	}
 }
 
@@ -253,20 +253,18 @@  unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
 }
 
 /* Check if paddr is at an instruction boundary */
-static int can_probe(unsigned long paddr)
+static bool can_probe(unsigned long paddr)
 {
 	unsigned long addr, __addr, offset = 0;
 	struct insn insn;
 	kprobe_opcode_t buf[MAX_INSN_SIZE];
 
 	if (!kallsyms_lookup_size_offset(paddr, NULL, &offset))
-		return 0;
+		return false;
 
 	/* Decode instructions */
 	addr = paddr - offset;
 	while (addr < paddr) {
-		int ret;
-
 		/*
 		 * Check if the instruction has been modified by another
 		 * kprobe, in which case we replace the breakpoint by the
@@ -277,11 +275,10 @@  static int can_probe(unsigned long paddr)
 		 */
 		__addr = recover_probed_instruction(buf, addr);
 		if (!__addr)
-			return 0;
+			return false;
 
-		ret = insn_decode_kernel(&insn, (void *)__addr);
-		if (ret < 0)
-			return 0;
+		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
+			return false;
 
 #ifdef CONFIG_KGDB
 		/*
@@ -290,7 +287,7 @@  static int can_probe(unsigned long paddr)
 		 */
 		if (insn.opcode.bytes[0] == INT3_INSN_OPCODE &&
 		    kgdb_has_hit_break(addr))
-			return 0;
+			return false;
 #endif
 		addr += insn.length;
 	}
@@ -310,10 +307,10 @@  static int can_probe(unsigned long paddr)
 		 */
 		__addr = recover_probed_instruction(buf, addr);
 		if (!__addr)
-			return 0;
+			return false;
 
 		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
-			return 0;
+			return false;
 
 		if (insn.opcode.value == 0xBA)
 			offset = 12;
@@ -324,7 +321,7 @@  static int can_probe(unsigned long paddr)
 
 		/* This movl/addl is used for decoding CFI. */
 		if (is_cfi_trap(addr + offset))
-			return 0;
+			return false;
 	}
 
 out: