linux-kernel - Re: [PATCH v2 5/5] uprobes: make uprobe

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240731165614.GI33588@noisy.programming.kicks-ass.net>
Date: Wed, 31 Jul 2024 18:56:14 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Oleg Nesterov <oleg@...hat.com>, andrii@...nel.org, mhiramat@...nel.org,
	jolsa@...nel.org, rostedt@...dmis.org, linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct
 uprobe *

On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:
> On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@...hat.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@...hat.com>
> > Acked-by: Andrii Nakryiko <andrii@...nel.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!

Do I stuff this on top of Oleg's patch or do you want me to fold it in
one of them?

> commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)
> Author: Andrii Nakryiko <andrii@...nel.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@...nel.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);
> 
> 
> [...]