On Tue, May 26, 2026 at 09:46:37PM +0000, [email protected] wrote:
> > commit 5ad96df841a0877eeca7d2cbe4b3f726b7fd0b72
> > Author: Jiri Olsa <[email protected]>
> > Date: Tue May 26 22:58:33 2026 +0200
> >
> > libbpf: Change has_nop_combo to work on top of nop10
> >
> > We now expect nop combo with 10 bytes nop instead of 5 bytes nop,
> > fixing has_nop_combo to reflect that.
> >
> > Fixes: 41a5c7df4466 ("libbpf: Add support to detect nop,nop5
> > instructions combo for usdt probe")
> > Reviewed-by: Jakub Sitnicki <[email protected]>
> > Acked-by: Andrii Nakryiko <[email protected]>
> > Signed-off-by: Jiri Olsa <[email protected]>
>
> The Fixes: tag references commit 41a5c7df4466 which originally added
> has_nop_combo() with correct nop5 support. However, the actual bug was
> introduced by commit af734a7ddb4e which changed the kernel's uprobe
> implementation from nop5 to nop10. Should the tag be:
>
> Fixes: af734a7ddb4e ("uprobes/x86: Move optimized uprobe from nop5 to
> nop10")
af734a7ddb4e hash is not available, the patch:
uprobes/x86: Move optimized uprobe from nop5 to nop10
is part of this patchset
>
> > diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> > index e3710933fd52..484a4354e82b 100644
> > --- a/tools/lib/bpf/usdt.c
> > +++ b/tools/lib/bpf/usdt.c
> > @@ -305,7 +305,7 @@ struct usdt_manager *usdt_manager_new(struct bpf_object
> > *obj)
> >
> > /*
> > * Detect kernel support for uprobe() syscall, it's presence means we
> > can
> > - * take advantage of faster nop5 uprobe handling.
> > + * take advantage of faster nop10 uprobe handling.
> > * Added in: 56101b69c919 ("uprobes/x86: Add uprobe syscall to speed up
> > uprobe")
> > */
>
> The comment at the end of this block references commit 56101b69c919 which
> added the uprobe syscall. Andrii Nakryiko noted in v3 review that it would
> be good to also reference the commit that switches nop5 to nop10, though he
> acknowledged the hash isn't stable until it lands. Now that af734a7ddb4e is
> available, could this be updated to reference both commits?
I proposed to change the comment after this gets merged
>
> > @@ -596,14 +596,14 @@ static int parse_usdt_spec(struct usdt_spec *spec,
> > const struct usdt_note *note,
> > #if defined(__x86_64__)
> > static bool has_nop_combo(int fd, long off)
> > {
> > - unsigned char nop_combo[6] = {
> > - 0x90, 0x0f, 0x1f, 0x44, 0x00, 0x00 /* nop,nop5 */
> > + unsigned char nop_combo[11] = {
> > + 0x90, 0x66, 0x2e, 0x0f, 0x1f, 0x84, 0x00, 0x00, 0x00, 0x00,
> > 0x00,
> > };
> > - unsigned char buf[6];
> > + unsigned char buf[11];
> >
> > - if (pread(fd, buf, 6, off) != 6)
> > + if (pread(fd, buf, 11, off) != 11)
> > return false;
> > - return memcmp(buf, nop_combo, 6) == 0;
> > + return memcmp(buf, nop_combo, 11) == 0;
> > }
>
> This isn't a bug, but Jakub Sitnicki suggested in v1 review using
> ARRAY_SIZE(buf) instead of repeating the scalar value 11. The author
> preferred keeping the literal number for simplicity, though the value
> now appears in 5 locations (array declarations, pread size, pread check,
> memcmp size).
and I replied to that then
jirka
