Re: [PATCH v3 4/6] x86/alternatives: Add and use text_gen_insn() helper

[Masami Hiramatsu <mhiramat@kernel.org>]

On Mon, 07 Oct 2019 10:17:20 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Provide a simple helper function to create common instruction
> encodings.

Thanks for using correct INSN_OPCODE:)
This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/x86/include/asm/text-patching.h |    2 +
>  arch/x86/kernel/alternative.c        |   36 +++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/jump_label.c         |   31 ++++++++++--------------------
>  arch/x86/kernel/kprobes/opt.c        |    7 ------
>  4 files changed, 50 insertions(+), 26 deletions(-)
> 
> --- a/arch/x86/include/asm/text-patching.h
> +++ b/arch/x86/include/asm/text-patching.h
> @@ -49,6 +49,8 @@ extern void text_poke_bp(void *addr, con
>  extern void text_poke_queue(void *addr, const void *opcode, size_t len, const void *emulate);
>  extern void text_poke_finish(void);
>  
> +extern void *text_gen_insn(u8 opcode, const void *addr, const void *dest);
> +
>  extern int after_bootmem;
>  extern __ro_after_init struct mm_struct *poking_mm;
>  extern __ro_after_init unsigned long poking_addr;
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1237,3 +1237,39 @@ void text_poke_bp(void *addr, const void
>  	text_poke_loc_init(&tp, addr, opcode, len, emulate);
>  	text_poke_bp_batch(&tp, 1);
>  }
> +
> +union text_poke_insn {
> +	u8 text[POKE_MAX_OPCODE_SIZE];
> +	struct {
> +		u8 opcode;
> +		s32 disp;
> +	} __attribute__((packed));
> +};
> +
> +void *text_gen_insn(u8 opcode, const void *addr, const void *dest)
> +{
> +	static union text_poke_insn insn; /* text_mutex */
> +	int size = 0;
> +
> +	lockdep_assert_held(&text_mutex);
> +
> +	insn.opcode = opcode;
> +
> +#define __CASE(insn)	\
> +	case insn##_INSN_OPCODE: size = insn##_INSN_SIZE; break
> +
> +	switch(opcode) {
> +	__CASE(INT3);
> +	__CASE(CALL);
> +	__CASE(JMP32);
> +	__CASE(JMP8);
> +	}
> +
> +	if (size > 1) {
> +		insn.disp = (long)dest - (long)(addr + size);
> +		if (size == 2)
> +			BUG_ON((insn.disp >> 31) != (insn.disp >> 7));
> +	}
> +
> +	return &insn.text;
> +}
> --- a/arch/x86/kernel/jump_label.c
> +++ b/arch/x86/kernel/jump_label.c
> @@ -16,15 +16,7 @@
>  #include <asm/alternative.h>
>  #include <asm/text-patching.h>
>  
> -union jump_code_union {
> -	char code[JUMP_LABEL_NOP_SIZE];
> -	struct {
> -		char jump;
> -		int offset;
> -	} __attribute__((packed));
> -};
> -
> -static void bug_at(unsigned char *ip, int line)
> +static void bug_at(const void *ip, int line)
>  {
>  	/*
>  	 * The location is not an op that we were expecting.
> @@ -38,33 +30,32 @@ static void bug_at(unsigned char *ip, in
>  static const void *
>  __jump_label_set_jump_code(struct jump_entry *entry, enum jump_label_type type, int init)
>  {
> -	static union jump_code_union code; /* relies on text_mutex */
>  	const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
>  	const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
> -	const void *expect;
> +	const void *expect, *code;
> +	const void *addr, *dest;
>  	int line;
>  
> -	lockdep_assert_held(&text_mutex);
> +	addr = (void *)jump_entry_code(entry);
> +	dest = (void *)jump_entry_target(entry);
>  
> -	code.jump = JMP32_INSN_OPCODE;
> -	code.offset = jump_entry_target(entry) -
> -		       (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
> +	code = text_gen_insn(JMP32_INSN_OPCODE, addr, dest);
>  
>  	if (init) {
>  		expect = default_nop; line = __LINE__;
>  	} else if (type == JUMP_LABEL_JMP) {
>  		expect = ideal_nop; line = __LINE__;
>  	} else {
> -		expect = code.code; line = __LINE__;
> +		expect = code; line = __LINE__;
>  	}
>  
> -	if (memcmp((void *)jump_entry_code(entry), expect, JUMP_LABEL_NOP_SIZE))
> -		bug_at((void *)jump_entry_code(entry), line);
> +	if (memcmp(addr, expect, JUMP_LABEL_NOP_SIZE))
> +		bug_at(addr, line);
>  
>  	if (type == JUMP_LABEL_NOP)
> -		memcpy(&code, ideal_nop, JUMP_LABEL_NOP_SIZE);
> +		code = ideal_nop;
>  
> -	return &code;
> +	return code;
>  }
>  
>  static void inline __jump_label_transform(struct jump_entry *entry,
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -447,18 +447,13 @@ void arch_optimize_kprobes(struct list_h
>  void arch_unoptimize_kprobe(struct optimized_kprobe *op)
>  {
>  	u8 insn_buff[RELATIVEJUMP_SIZE];
> -	u8 emulate_buff[RELATIVEJUMP_SIZE];
>  
>  	/* Set int3 to first byte for kprobes */
>  	insn_buff[0] = BREAKPOINT_INSTRUCTION;
>  	memcpy(insn_buff + 1, op->optinsn.copied_insn, RELATIVE_ADDR_SIZE);
>  
> -	emulate_buff[0] = RELATIVEJUMP_OPCODE;
> -	*(s32 *)(&emulate_buff[1]) = (s32)((long)op->optinsn.insn -
> -			((long)op->kp.addr + RELATIVEJUMP_SIZE));
> -
>  	text_poke_bp(op->kp.addr, insn_buff, RELATIVEJUMP_SIZE,
> -		     emulate_buff);
> +		     text_gen_insn(JMP32_INSN_OPCODE, op->kp.addr, op->optinsn.insn));
>  }
>  
>  /*
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>