netdev - Re: [GIT PULL] BPF changes for 6.18

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aN0JVRynHxqKy4lw@krava>
Date: Wed, 1 Oct 2025 12:58:29 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, bpf@...r.kernel.org,
	daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org,
	peterz@...radead.org, kuba@...nel.org, linux-kernel@...r.kernel.org,
	mingo@...nel.org, netdev@...r.kernel.org
Subject: Re: [GIT PULL] BPF changes for 6.18

On Tue, Sep 30, 2025 at 07:09:43PM -0700, Linus Torvalds wrote:
> [ Jiri added to participants ]
> 
> On Sun, 28 Sept 2025 at 08:46, Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > Note, there is a trivial conflict between tip and bpf-next trees:
> > in kernel/events/uprobes.c between commit:
> >   4363264111e12 ("uprobe: Do not emulate/sstep original instruction when ip is changed")
> > from the bpf-next tree and commit:
> >   ba2bfc97b4629 ("uprobes/x86: Add support to optimize uprobes")
> > from the tip tree:
> > https://lore.kernel.org/all/aNVMR5rjA2geHNLn@sirena.org.uk/
> > since Jiri's two separate uprobe/bpf related patch series landed
> > in different trees. One was mostly uprobe. Another was mostly bpf.
> 
> So the conflict isn't complicated and I did it the way linux-next did
> it, but honestly, the placement of that arch_uprobe_optimize() thing
> isn't obvious.
> 
> My first reaction was to put it before the instruction_pointer()
> check, because it seems like whatever rewriting the arch wants to do
> might as well be done regardless.
> 
> It's very confusing how it's sometimes skipped, and sometimes not
> skipped. For example. if the uprobe is skipped because of
> single-stepping disabling it, the arch optimization still *will* be
> done, because the "skip_sstep()" test is done after - but other
> skipping tests are done before.
> 
> Jiri, it would be good to just add a note about when that optimization
> is done and when not done. Because as-is, it's very confusing.
> 
> The answer may well be "it doesn't matter, semantics are the same" (I
> suspect that _is_ the answer), but even so that current ordering is
> just confusing when it sometimes goes through that
> arch_uprobe_optimize() and sometimes skips it.

yes, either way will work fine, but perhaps the other way round to
first optimize and then skip uprobe if needed is less confusing

> 
> Side note: the conflict in the selftests was worse, and the magic to
> build it is not obvious. It errors out randomly with various kernel
> configs with useless error messages, and I eventually just gave up
> entirely with a
> 
>    attempt to use poisoned ‘gettid’
> 
> error.
> 
>              Linus

I ended up with changes below, should I send formal patches?

thanks,
jirka


---
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 5dcf927310fd..c14ec27b976d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2765,6 +2765,9 @@ static void handle_swbp(struct pt_regs *regs)
 
 	handler_chain(uprobe, regs);
 
+	/* Try to optimize after first hit. */
+	arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
+
 	/*
 	 * If user decided to take execution elsewhere, it makes little sense
 	 * to execute the original instruction, so let's skip it.
@@ -2772,9 +2775,6 @@ static void handle_swbp(struct pt_regs *regs)
 	if (instruction_pointer(regs) != bp_vaddr)
 		goto out;
 
-	/* Try to optimize after first hit. */
-	arch_uprobe_optimize(&uprobe->arch, bp_vaddr);
-
 	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 		goto out;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 6d75ede16e7c..955a37751b52 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -661,7 +661,7 @@ static void *worker_trigger(void *arg)
 		rounds++;
 	}
 
-	printf("tid %d trigger rounds: %lu\n", gettid(), rounds);
+	printf("tid %ld trigger rounds: %lu\n", sys_gettid(), rounds);
 	return NULL;
 }
 
@@ -704,7 +704,7 @@ static void *worker_attach(void *arg)
 		rounds++;
 	}
 
-	printf("tid %d attach rounds: %lu hits: %d\n", gettid(), rounds, skel->bss->executed);
+	printf("tid %ld attach rounds: %lu hits: %d\n", sys_gettid(), rounds, skel->bss->executed);
 	uprobe_syscall_executed__destroy(skel);
 	free(ref);
 	return NULL;
diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
index 4f7f45e69315..f4be5269fa90 100644
--- a/tools/testing/selftests/bpf/prog_tests/usdt.c
+++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
@@ -142,7 +142,7 @@ static void subtest_basic_usdt(bool optimized)
 		goto cleanup;
 #endif
 
-	alled = TRIGGER(1);
+	called = TRIGGER(1);
 
 	ASSERT_EQ(bss->usdt0_called, called, "usdt0_called");
 	ASSERT_EQ(bss->usdt3_called, called, "usdt3_called");