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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Apr 09, 2025 at 11:19:36AM -0700, Andrii Nakryiko wrote:
> On Tue, Apr 8, 2025 at 1:22 PM Jiri Olsa <olsajiri@gmail.com> 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@kernel.org> wrote:
> > > > >
> > > > > Adding support to emulate nop5 as the original uprobe instruction.
> > > > >
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  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).

ok, Oleg suggested similar change, I sent v2 with that

thanks,
jirka

> 
> 
> > 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 */