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