Re: [PATCHv4 11/13] selftests/bpf: Add reattach tests for uprobe syscall

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, May 27, 2026 at 01:32:48PM +0200, Jakub Sitnicki wrote:
> On Tue, May 26, 2026 at 10:58 PM +02, Jiri Olsa wrote:
> > Adding reattach tests for uprobe syscall tests to make sure
> > we can re-attach and optimize same uprobe multiple times.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/uprobe_syscall.c | 114 ++++++++++++++++--
> >  1 file changed, 104 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > index 0868fb9793e0..969f4deba9fd 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > @@ -430,23 +430,28 @@ static void *check_attach(struct uprobe_syscall_executed *skel, trigger_t trigge
> >  	return tramp;
> >  }
> >  
> > -static void check_detach(void *addr, void *tramp)
> > +static bool check_detach(void *addr, void *tramp)
> >  {
> >  	static const char nop10_prefix[] = { 0x66, 0x2e, 0x0f, 0x1f, 0x84 };
> > +	bool ok = true;
> >  
> >  	/* [uprobes_trampoline] stays after detach */
> > -	ASSERT_OK(find_uprobes_trampoline(tramp), "uprobes_trampoline");
> > -	ASSERT_OK(memcmp(addr, nop10_prefix, 5), "nop10_prefix");
> > +	if (!ASSERT_OK(find_uprobes_trampoline(tramp), "uprobes_trampoline"))
> > +		ok = false;
> > +	if (!ASSERT_OK(memcmp(addr, nop10_prefix, 5), "nop10_prefix"))
> > +		ok = false;
> > +	return ok;
> >  }
> 
> Nit: Maybe apply the same pattern you used in
> progs/get_func_args_test.c?
> 
> ok &= ASSERT_OK(...)
> ok &= ASSERT_OK(...)

ok, safes space ;-)

> 
> >  
> > -static void check(struct uprobe_syscall_executed *skel, struct bpf_link *link,
> > -		  trigger_t trigger, void *addr, int executed)
> > +static void *check(struct uprobe_syscall_executed *skel, struct bpf_link *link,
> > +		   trigger_t trigger, void *addr, int executed)
> 
> Nit: Kinda wish that was called check_attach_detach().

would need to check the existing code.. let's keep it

> 
> >  {
> >  	void *tramp;
> >  
> >  	tramp = check_attach(skel, trigger, addr, executed);
> >  	bpf_link__destroy(link);
> >  	check_detach(addr, tramp);
> > +	return tramp;
> >  }
> >  
> >  static void test_uprobe_legacy(void)
> > @@ -457,6 +462,7 @@ static void test_uprobe_legacy(void)
> >  	);
> >  	struct bpf_link *link;
> >  	unsigned long offset;
> > +	void *tramp;
> >  
> >  	offset = get_uprobe_offset(&uprobe_test);
> >  	if (!ASSERT_GE(offset, 0, "get_uprobe_offset"))
> > @@ -474,7 +480,28 @@ static void test_uprobe_legacy(void)
> >  	if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_opts"))
> >  		goto cleanup;
> >  
> > -	check(skel, link, uprobe_test, uprobe_test, 2);
> > +	tramp = check(skel, link, uprobe_test, uprobe_test, 2);
> > +
> > +	/* reattach and detach without triggering optimization */
> > +	link = bpf_program__attach_uprobe_opts(skel->progs.test_uprobe,
> > +					       0, "/proc/self/exe", offset, NULL);
> > +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_opts"))
> > +		goto cleanup;
> > +
> 
> In theory we're missing a check here that an unoptimized uprobe was
> installed. If nothing happened at all between the last check() and
> check_destroy() below, the test would still pass.
> 
> Applies to the three similar changes after that one as well.

right, we can add extra check that there's int3 in here (and below)

thanks,
jirka