Message-ID: <CAEf4BzZa0Ye83QfTbw6Sw3ERg2PJ7ioN_pEFHYui6JGEHhOg4Q@mail.gmail.com>
Date: Wed, 10 Jul 2024 11:21:47 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: andrii@...nel.org, mhiramat@...nel.org, peterz@...radead.org, clm@...a.com, 
	jolsa@...nel.org, mingo@...nel.org, paulmck@...nel.org, rostedt@...dmis.org, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] uprobes: make uprobe_register() return struct uprobe *

On Wed, Jul 10, 2024 at 9:33 AM Oleg Nesterov <oleg@...hat.com> wrote:
>
> This way uprobe_unregister() and uprobe_apply() do not need find_uprobe() +
> put_uprobe(). And to me this change simplifies the code a bit.
>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  include/linux/uprobes.h     | 14 ++++++------
>  kernel/events/uprobes.c     | 45 ++++++++++++-------------------------
>  kernel/trace/bpf_trace.c    | 12 +++++-----
>  kernel/trace/trace_uprobe.c | 28 +++++++++++------------
>  4 files changed, 41 insertions(+), 58 deletions(-)
>
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index aa89a8b67039..399509befcf4 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h

I don't see struct uprobe forward-declared in this header, maybe we
should add it?

> @@ -110,9 +110,9 @@ extern bool is_trap_insn(uprobe_opcode_t *insn);
>  extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
>  extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
>  extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
> -extern int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
> -extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
> -extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> +extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
> +extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool);
> +extern void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc);
>  extern int uprobe_mmap(struct vm_area_struct *vma);
>  extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
>  extern void uprobe_start_dup_mmap(void);
> @@ -147,18 +147,18 @@ static inline void uprobes_init(void)
>
>  #define uprobe_get_trap_addr(regs)     instruction_pointer(regs)
>
> -static inline int
> +static inline struct uprobe *
>  uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc)
>  {
> -       return -ENOSYS;
> +       return ERR_PTR(-ENOSYS);
>  }
>  static inline int
> -uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool add)
> +uprobe_apply(struct uprobe* uprobe, struct uprobe_consumer *uc, bool add)
>  {
>         return -ENOSYS;
>  }

complete aside, when I was looking at this code I was wondering why we
even need uprobe_apply, it looks like some hacky variant of
uprobe_register and uprobe_unregister. I didn't dig deeper, but think
whether we even need this? If this is just to avoid (for some period)
some consumer callback calling, then that could be handled at the
consumer side by ignoring such calls.

callback call is cheap, it's the int3 handling that's expensive and
with uprobe_apply we are already paying it anyways, so what is this
for?

>  static inline void
> -uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
> +uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
>  }
>  static inline int uprobe_mmap(struct vm_area_struct *vma)

[...]

>
> @@ -1133,41 +1126,39 @@ EXPORT_SYMBOL_GPL(uprobe_unregister);
>   * refcount is released when the last @uc for the @uprobe
>   * unregisters. Caller of uprobe_register() is required to keep @inode
>   * (and the containing mount) referenced.
> - *
> - * Return errno if it cannot successully install probes
> - * else return 0 (success)

mention that it never returns NULL, but rather encodes error code
inside the pointer on error? It's an important part of the contract.

>   */
> -int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset,
> -                   struct uprobe_consumer *uc)
> +struct uprobe *uprobe_register(struct inode *inode,
> +                               loff_t offset, loff_t ref_ctr_offset,
> +                               struct uprobe_consumer *uc)
>  {

[...]

> @@ -1186,35 +1177,27 @@ int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset,
>
>         if (unlikely(ret == -EAGAIN))
>                 goto retry;
> -       return ret;
> +
> +       return ret ? ERR_PTR(ret) : uprobe;
>  }
>  EXPORT_SYMBOL_GPL(uprobe_register);
>
>  /*

this should be /** for doccomment checking (you'd get a warning for
missing @uprobe if there was this extra star)

>   * uprobe_apply - unregister an already registered probe.
> - * @inode: the file in which the probe has to be removed.
> - * @offset: offset from the start of the file.

add @uprobe description now?

>   * @uc: consumer which wants to add more or remove some breakpoints
>   * @add: add or remove the breakpoints
>   */
> -int uprobe_apply(struct inode *inode, loff_t offset,
> -                       struct uprobe_consumer *uc, bool add)
> +int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool add)
>  {
> -       struct uprobe *uprobe;
>         struct uprobe_consumer *con;
>         int ret = -ENOENT;
>

[...]

> @@ -3180,10 +3181,8 @@ static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes,
>  {
>         u32 i;
>
> -       for (i = 0; i < cnt; i++) {
> -               uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset,
> -                                 &uprobes[i].consumer);
> -       }
> +       for (i = 0; i < cnt; i++)

you'll now need !IS_ERR_OR_NULL(uprobes[i].uprobe) check (or just NULL
check if you null-out it below)

> +               uprobe_unregister(uprobes[i].uprobe, &uprobes[i].consumer);
>  }
>
>  static void bpf_uprobe_multi_link_release(struct bpf_link *link)
> @@ -3477,11 +3476,12 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                       &bpf_uprobe_multi_link_lops, prog);
>
>         for (i = 0; i < cnt; i++) {
> -               err = uprobe_register(d_real_inode(link->path.dentry),
> +               uprobes[i].uprobe = uprobe_register(d_real_inode(link->path.dentry),

will uprobe keep inode alive as long as uprobe is attached? If that's
the case we can get rid of link->path (have it only as a local
variable which we put as soon as we are done with registration). We
can probably do that clean up separately, I'll defer to Jiri.

>                                              uprobes[i].offset,
>                                              uprobes[i].ref_ctr_offset,
>                                              &uprobes[i].consumer);
> -               if (err) {
> +               if (IS_ERR(uprobes[i].uprobe)) {
> +                       err = PTR_ERR(uprobes[i].uprobe);

we can NULL-out uprobe on error for bpf_uprobe_unregister() to handle
only NULL vs non-NULL case

or maybe better yet let's just have local struct uprobe variable and
only assign it if registration succeeded (still need NULL check in
bpf_uprobe_unregister above)

>                         bpf_uprobe_unregister(&path, uprobes, i);
>                         goto error_free;
>                 }

[...]

Powered by blists - more mailing lists