Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

Andrii Nakryiko
2024-07-31T16:18:13Z

On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *"
> rather than inode + offset. This simplifies the code and allows to avoid
> the unnecessary find_uprobe() + put_uprobe() in these functions.
>
> TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that
> this uprobe can't be freed before up_write(&uprobe->register_rwsem).
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> include/linux/uprobes.h | 15 +++++-----
> kernel/events/uprobes.c | 56 +++++++++++++++----------------------
> kernel/trace/bpf_trace.c | 25 ++++++++---------
> kernel/trace/trace_uprobe.c | 26 ++++++++---------
> 4 files changed, 55 insertions(+), 67 deletions(-)
>

You'll need something like below to not break our bpf_testmod. And
please send pull patch sets, not individually updated patches, it's a
PITA to deal with. Thanks!

commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)
Author: Andrii Nakryiko <andrii@kernel.org>
Date: Wed Jul 31 09:15:46 2024 -0700

uprobes: fix bpf_testmod after uprobe_register/uprobe_unregister API change

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 5f152afdec2f..73a6b041bcce 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -431,6 +431,7 @@ uprobe_ret_handler(struct uprobe_consumer *self,
unsigned long func,
}

struct testmod_uprobe {
+ struct uprobe *uprobe;
struct path path;
loff_t offset;
struct uprobe_consumer consumer;
@@ -458,12 +459,14 @@ static int testmod_register_uprobe(loff_t offset)
if (err)
goto out;

- err = uprobe_register(d_real_inode(uprobe.path.dentry),
- offset, 0, &uprobe.consumer);
- if (err)
+ uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
+ offset, 0, &uprobe.consumer);
+ if (IS_ERR(uprobe.uprobe)) {
path_put(&uprobe.path);
- else
+ uprobe.uprobe = NULL;
+ } else {
uprobe.offset = offset;
+ }

out:
mutex_unlock(&testmod_uprobe_mutex);
@@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
{
mutex_lock(&testmod_uprobe_mutex);

- if (uprobe.offset) {
- uprobe_unregister(d_real_inode(uprobe.path.dentry),
- uprobe.offset, &uprobe.consumer);
+ if (uprobe.uprobe) {
+ uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
uprobe.offset = 0;
+ uprobe.uprobe = NULL;
}

mutex_unlock(&testmod_uprobe_mutex); …