Re: [PATCHv4 06/13] libbpf: Change has_nop_combo to work on top of nop10

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, May 26, 2026 at 09:46:37PM +0000, bot+bpf-ci@kernel.org wrote:
> > commit 5ad96df841a0877eeca7d2cbe4b3f726b7fd0b72
> > Author: Jiri Olsa <jolsa@kernel.org>
> > 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 <jakub@cloudflare.com>
> >     Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >     Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> 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