| Message ID | 20250408211310.51491-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] uprobes/x86: Add support to emulate nop5 instruction | expand |
On 04/08, Jiri Olsa wrote: > > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -608,6 +608,16 @@ static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > *sr = utask->autask.saved_scratch_register; > } > } > + > +static int is_nop5_insn(uprobe_opcode_t *insn) > +{ > + return !memcmp(insn, x86_nops[5], 5); > +} > + > +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) > +{ > + return is_nop5_insn((uprobe_opcode_t *) &auprobe->insn); > +} Why do we need 2 functions? Can't branch_setup_xol_ops() just use is_nop5_insn(insn->kaddr) ? > #else /* 32-bit: */ > /* > * No RIP-relative addressing on 32-bit > @@ -621,6 +631,10 @@ static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > } > +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) > +{ > + return false; > +} Hmm, why? I mean, why we can't emulate x86_nops[5] if !CONFIG_X86_64 ? OTOH. What if the kernel is 64-bit, but the probed task is 32-bit and it uses the 64-bit version of BYTES_NOP5? Perhaps this is fine, I simply don't know, so let me ask... > @@ -852,6 +866,8 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > break; > > case 0x0f: > + if (emulate_nop5_insn(auprobe)) > + goto setup; I think this will work, but if we want to emulate nop5, then perhaps we can do the same for other nops? For the moment, lets forget about compat tasks on a 64-bit kernel, can't we simply do something like below? Oleg. --- diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index 9194695662b2..76d2cceca6c4 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -840,12 +840,16 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) insn_byte_t p; int i; + /* prefix* + nop[i]; same as jmp with .offs = 0 */ + for (i = 1; i <= ASM_NOP_MAX; ++i) { + if (!memcmp(insn->kaddr, x86_nops[i], i)) + goto setup; + } + switch (opc1) { case 0xeb: /* jmp 8 */ case 0xe9: /* jmp 32 */ break; - case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */ - goto setup; case 0xe8: /* call relative */ branch_clear_offset(auprobe, insn);
On 04/09, Oleg Nesterov wrote: > > For the moment, lets forget about compat tasks on a 64-bit kernel, can't > we simply do something like below? ... > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -840,12 +840,16 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > insn_byte_t p; > int i; > > + /* prefix* + nop[i]; same as jmp with .offs = 0 */ > + for (i = 1; i <= ASM_NOP_MAX; ++i) { > + if (!memcmp(insn->kaddr, x86_nops[i], i)) > + goto setup; > + } > + > switch (opc1) { > case 0xeb: /* jmp 8 */ > case 0xe9: /* jmp 32 */ > break; > - case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */ > - goto setup; OK, I guess we can't remove this "case 0x90" because of prefixes, please ignore this part. Oleg.
On Wed, Apr 09, 2025 at 01:28:39PM +0200, Oleg Nesterov wrote: > On 04/08, Jiri Olsa wrote: > > > > --- a/arch/x86/kernel/uprobes.c > > +++ b/arch/x86/kernel/uprobes.c > > @@ -608,6 +608,16 @@ static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > > *sr = utask->autask.saved_scratch_register; > > } > > } > > + > > +static int is_nop5_insn(uprobe_opcode_t *insn) > > +{ > > + return !memcmp(insn, x86_nops[5], 5); > > +} > > + > > +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) > > +{ > > + return is_nop5_insn((uprobe_opcode_t *) &auprobe->insn); > > +} > > Why do we need 2 functions? Can't branch_setup_xol_ops() just use > is_nop5_insn(insn->kaddr) ? I need is_nop5_insn in other changes I have in queue, so did not want to introduce extra changes > > > #else /* 32-bit: */ > > /* > > * No RIP-relative addressing on 32-bit > > @@ -621,6 +631,10 @@ static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > > static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > > { > > } > > +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) > > +{ > > + return false; > > +} > > Hmm, why? I mean, why we can't emulate x86_nops[5] if !CONFIG_X86_64 ? ok, so the following uprobe optimization is for CONFIG_X86_64 only, so I followed that, but I guess we might emulate nop5 for !CONFIG_X86_64 > > OTOH. What if the kernel is 64-bit, but the probed task is 32-bit and it > uses the 64-bit version of BYTES_NOP5? > > Perhaps this is fine, I simply don't know, so let me ask... hum, did not think of that, let me try it > > > @@ -852,6 +866,8 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > > break; > > > > case 0x0f: > > + if (emulate_nop5_insn(auprobe)) > > + goto setup; > > I think this will work, but if we want to emulate nop5, then perhaps > we can do the same for other nops? > > For the moment, lets forget about compat tasks on a 64-bit kernel, can't > we simply do something like below? I sent similar change (CONFIG_X86_64 only) in this thread: https://lore.kernel.org/bpf/Z_O0Z1ON1YlRqyny@krava/T/#m59c430fb5a30cb9faeb9587fd672ea0adbf3ef4f uprobe won't attach on nop9/10/11 atm, also I don't have practical justification for doing that.. nop5 seems to have future, because of the optimization thanks, jirka > > Oleg. > --- > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index 9194695662b2..76d2cceca6c4 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -840,12 +840,16 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > insn_byte_t p; > int i; > > + /* prefix* + nop[i]; same as jmp with .offs = 0 */ > + for (i = 1; i <= ASM_NOP_MAX; ++i) { > + if (!memcmp(insn->kaddr, x86_nops[i], i)) > + goto setup; > + } > + > switch (opc1) { > case 0xeb: /* jmp 8 */ > case 0xe9: /* jmp 32 */ > break; > - case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */ > - goto setup; > > case 0xe8: /* call relative */ > branch_clear_offset(auprobe, insn); >
On 04/09, Jiri Olsa wrote: > > On Wed, Apr 09, 2025 at 01:28:39PM +0200, Oleg Nesterov wrote: > > On 04/08, Jiri Olsa wrote: > > > > > > --- a/arch/x86/kernel/uprobes.c > > > +++ b/arch/x86/kernel/uprobes.c > > > @@ -608,6 +608,16 @@ static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > > > *sr = utask->autask.saved_scratch_register; > > > } > > > } > > > + > > > +static int is_nop5_insn(uprobe_opcode_t *insn) > > > +{ > > > + return !memcmp(insn, x86_nops[5], 5); > > > +} > > > + > > > +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) > > > +{ > > > + return is_nop5_insn((uprobe_opcode_t *) &auprobe->insn); > > > +} > > > > Why do we need 2 functions? Can't branch_setup_xol_ops() just use > > is_nop5_insn(insn->kaddr) ? > > I need is_nop5_insn in other changes I have in queue, so did not want > to introduce extra changes But I didn't suggest to remove is_nop5_insn(), I meant that branch_setup_xol_ops() can do if (is_nop5_insn(insn->kaddr)) goto setup; or if (is_nop5_insn(auprobe->insn)) goto setup; this even looks more readable to me. but I won't insist. > > For the moment, lets forget about compat tasks on a 64-bit kernel, can't > > we simply do something like below? > > I sent similar change (CONFIG_X86_64 only) in this thread: > https://lore.kernel.org/bpf/Z_O0Z1ON1YlRqyny@krava/T/#m59c430fb5a30cb9faeb9587fd672ea0adbf3ef4f > > uprobe won't attach on nop9/10/11 atm, Ah, OK, I didn't know. But this means that nop9/10/11 will be rejected by uprobe_init_insn() -> is_prefix_bad() before branch_setup_xol_ops() is called, right? So I guess it is safe to use ASM_NOP_MAX. Nevermind. > also I don't have practical justification > for doing that.. nop5 seems to have future, because of the optimization OK, I won't insist, up to you. Just it looks a bit strange to me. Even if we do not have a use-case for other nops, why we can't emulate them all just for consistency? And given that emulate_nop5_insn() compares the whole insn with x86_nops[5], I guess we don't even need to check OPCODE1(insn)... Nevermind. So, once again, I won't argue. Oleg.
On Wed, Apr 09, 2025 at 03:11:16PM +0200, Oleg Nesterov wrote: > On 04/09, Jiri Olsa wrote: > > > > On Wed, Apr 09, 2025 at 01:28:39PM +0200, Oleg Nesterov wrote: > > > On 04/08, Jiri Olsa wrote: > > > > > > > > --- a/arch/x86/kernel/uprobes.c > > > > +++ b/arch/x86/kernel/uprobes.c > > > > @@ -608,6 +608,16 @@ static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > > > > *sr = utask->autask.saved_scratch_register; > > > > } > > > > } > > > > + > > > > +static int is_nop5_insn(uprobe_opcode_t *insn) > > > > +{ > > > > + return !memcmp(insn, x86_nops[5], 5); > > > > +} > > > > + > > > > +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) > > > > +{ > > > > + return is_nop5_insn((uprobe_opcode_t *) &auprobe->insn); > > > > +} > > > > > > Why do we need 2 functions? Can't branch_setup_xol_ops() just use > > > is_nop5_insn(insn->kaddr) ? > > > > I need is_nop5_insn in other changes I have in queue, so did not want > > to introduce extra changes > > But I didn't suggest to remove is_nop5_insn(), I meant that > branch_setup_xol_ops() can do > > if (is_nop5_insn(insn->kaddr)) > goto setup; > or > if (is_nop5_insn(auprobe->insn)) > goto setup; > > this even looks more readable to me. but I won't insist. > > > > For the moment, lets forget about compat tasks on a 64-bit kernel, can't > > > we simply do something like below? > > > > I sent similar change (CONFIG_X86_64 only) in this thread: > > https://lore.kernel.org/bpf/Z_O0Z1ON1YlRqyny@krava/T/#m59c430fb5a30cb9faeb9587fd672ea0adbf3ef4f > > > > uprobe won't attach on nop9/10/11 atm, > > Ah, OK, I didn't know. But this means that nop9/10/11 will be rejected > by uprobe_init_insn() -> is_prefix_bad() before branch_setup_xol_ops() is > called, right? So I guess it is safe to use ASM_NOP_MAX. Nevermind. > > > also I don't have practical justification > > for doing that.. nop5 seems to have future, because of the optimization > > OK, I won't insist, up to you. > > Just it looks a bit strange to me. Even if we do not have a use-case > for other nops, why we can't emulate them all just for consistency? we can, I went with nop5 just for simplicity, if you think having all nops support is better, let's do that I checked and compact process executes 64bit nops just fine, so we should be ok there > > And given that emulate_nop5_insn() compares the whole insn with > x86_nops[5], I guess we don't even need to check OPCODE1(insn)... right > Nevermind. > > So, once again, I won't argue. I'm happy to go with your version, wdyt? thanks, jirka --- diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index 9194695662b2..63ecc5f6c235 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -840,12 +840,16 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) insn_byte_t p; int i; + /* x86_nops[i]; same as jmp with .offs = 0 */ + for (i = 1; i <= ASM_NOP_MAX; ++i) { + if (!memcmp(insn->kaddr, x86_nops[i], i)) + goto setup; + } + switch (opc1) { case 0xeb: /* jmp 8 */ case 0xe9: /* jmp 32 */ break; - case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */ - goto setup; case 0xe8: /* call relative */ branch_clear_offset(auprobe, insn);
On 04/09, Jiri Olsa wrote: > > > Just it looks a bit strange to me. Even if we do not have a use-case > > for other nops, why we can't emulate them all just for consistency? > > we can, I went with nop5 just for simplicity, if you think > having all nops support is better, let's do that Well... Let me repeat, I am not really arguing and I do not want to delay your next changes. We can always cleanup this code later. Please see below. > I checked and compact process executes 64bit nops just fine, > so we should be ok there OK. Then, for your original patch: Acked-by: Oleg Nesterov <oleg@redhat.com> I'd only ask to define is_nop5_insn/emulate_nop5_insn regardless of CONFIG_X86_64. I understand that we have no reason to emulate nop5 on the 32-bit kernel, but at the same time I don't see any reason to complicate this code to explicitly "nack" nop5 in this case. As for the new version below: > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -840,12 +840,16 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) > insn_byte_t p; > int i; > > + /* x86_nops[i]; same as jmp with .offs = 0 */ > + for (i = 1; i <= ASM_NOP_MAX; ++i) { > + if (!memcmp(insn->kaddr, x86_nops[i], i)) > + goto setup; > + } Well, yes, I'd personally obviously prefer this version ;) Just because it looks a bit more clear/consistent to me. But this is subjective. And, > - case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */ > - goto setup; No, this is wrong. Please see my reply to myself, https://lore.kernel.org/all/20250409114950.GB32748@redhat.com/ This way we can no longer emulate, say, "rep; nop". Exactly because either way memcmp(x86_nops[i]) checks the whole instruction. Probably we don't really care, but still this patch shouldn't add any "regression". So, let me repeat. Up to you. Whatever you prefer. I just tried to understand your patch. You have my ACK in any case. Oleg.
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index 9194695662b2..63cc68e19da6 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -608,6 +608,16 @@ static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) *sr = utask->autask.saved_scratch_register; } } + +static int is_nop5_insn(uprobe_opcode_t *insn) +{ + return !memcmp(insn, x86_nops[5], 5); +} + +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) +{ + return is_nop5_insn((uprobe_opcode_t *) &auprobe->insn); +} #else /* 32-bit: */ /* * No RIP-relative addressing on 32-bit @@ -621,6 +631,10 @@ static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) { } +static bool emulate_nop5_insn(struct arch_uprobe *auprobe) +{ + return false; +} #endif /* CONFIG_X86_64 */ struct uprobe_xol_ops { @@ -852,6 +866,8 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) break; case 0x0f: + if (emulate_nop5_insn(auprobe)) + goto setup; if (insn->opcode.nbytes != 2) return -ENOSYS; /*
Adding support to emulate nop5 as the original uprobe instruction. This change speeds up uprobe on top of nop5 and is a preparation for usdt probe optimization, that will be done on top of nop5 instruction. With this change the usdt probe on top of nop5 won't take the performance hit compared to usdt probe on top of standard nop instruction. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- arch/x86/kernel/uprobes.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)