Re: [PATCH RFCv3 10/23] uprobes/x86: Add support to emulate nop5 instruction

[Andrii Nakryiko]

On Tue, Apr 8, 2025 at 1:22 PM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Mon, Apr 07, 2025 at 01:07:26PM +0200, Jiri Olsa wrote:
> > On Fri, Apr 04, 2025 at 01:33:11PM -0700, Andrii Nakryiko wrote:
> > > On Thu, Mar 20, 2025 at 4:43 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > > >
> > > > Adding support to emulate nop5 as the original uprobe instruction.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > ---
> > > >  arch/x86/kernel/uprobes.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > >
> > > This optimization is independent from the sys_uprobe, right? Maybe
> > > send it as a stand-alone patch and let's land it sooner?
> >
> > ok, will send it separately
> >
> > > Also, how hard would it be to do the same for other nopX instructions?
> >
> > will check, might be easy
>
> we can't do all at the moment, nop1-nop8 are fine, but uprobe won't
> attach on nop9/10/11 due unsupported prefix.. I guess insn decode
> would need to be updated first
>
> I'll send the nop5 emulation change, because of above and also I don't
> see practical justification to emulate other nops
>

Well, let me counter this approach: if we had nop5 emulation from the
day one, then we could have just transparently switched USDT libraries
to use nop5 because they would work well both before and after your
sys_uprobe changes. But we cannot, and that WILL cause problems and
headaches to work around that limitation.

See where I'm going with this? I understand the general "don't build
feature unless you have a use case", but in this case it's just a
matter of generality and common sense: we emulate nop1 and nop5, what
reasons do we have to not emulate all the other nops? Within reason,
of course. If it's hard to do some nopX, then it would be hard to
justify without a specific use case. But it doesn't seem so, at least
for nop1-nop8, so why not?

tl;dr, let's add all the nops we can emulate now, in one go, instead
of spoon-feeding this support through the years (with lots of
unnecessary backwards compatibility headaches associated with that
approach).


> jirka
>
>
> ---
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 9194695662b2..6616cc9866cc 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -608,6 +608,21 @@ static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>                 *sr = utask->autask.saved_scratch_register;
>         }
>  }
> +
> +static bool emulate_nop_insn(struct arch_uprobe *auprobe)
> +{
> +       unsigned int i;
> +
> +       /*
> +        * Uprobe is only allowed to be attached on nop1 through nop8. Further nop
> +        * instructions have unsupported prefix and uprobe fails to attach on them.
> +        */
> +       for (i = 1; i < 9; i++) {
> +               if (!memcmp(&auprobe->insn, x86_nops[i], i))
> +                       return true;
> +       }
> +       return false;
> +}
>  #else /* 32-bit: */
>  /*
>   * No RIP-relative addressing on 32-bit
> @@ -621,6 +636,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_nop_insn(struct arch_uprobe *auprobe)
> +{
> +       return false;
> +}
>  #endif /* CONFIG_X86_64 */
>
>  struct uprobe_xol_ops {
> @@ -840,6 +859,9 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>         insn_byte_t p;
>         int i;
>
> +       if (emulate_nop_insn(auprobe))
> +               goto setup;
> +
>         switch (opc1) {
>         case 0xeb:      /* jmp 8 */
>         case 0xe9:      /* jmp 32 */