Re: [PATCH 1/3] uprobes: kill uprobe_register_refctr()

[Andrii Nakryiko]

On Wed, Jul 10, 2024 at 9:32 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> It doesn't make any sense to have 2 versions of _register(). Note that
> trace_uprobe_enable(), the only user of uprobe_register(), doesn't need
> to check tu->ref_ctr_offset to decide which one should be used, it could
> safely pass ref_ctr_offset == 0 to uprobe_register_refctr().
>
> Add this argument to uprobe_register(), update the callers, and kill
> uprobe_register_refctr().
>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
>  include/linux/uprobes.h     |  9 ++-------
>  kernel/events/uprobes.c     | 23 +++++------------------
>  kernel/trace/bpf_trace.c    |  2 +-
>  kernel/trace/trace_uprobe.c |  8 ++------
>  4 files changed, 10 insertions(+), 32 deletions(-)
>

LGTM with few nits below.

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

>  /*
>   * uprobe_apply - unregister an already registered probe.
>   * @inode: the file in which the probe has to be removed.
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d1daeab1bbc1..467f358c8ce7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3477,7 +3477,7 @@ 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_refctr(d_real_inode(link->path.dentry),
> +               err = uprobe_register(d_real_inode(link->path.dentry),
>                                              uprobes[i].offset,
>                                              uprobes[i].ref_ctr_offset,
>                                              &uprobes[i].consumer);

please adjust indentation here

> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..78a5c40e885a 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1089,12 +1089,8 @@ static int trace_uprobe_enable(struct trace_uprobe *tu, filter_func_t filter)
>         tu->consumer.filter = filter;
>         tu->inode = d_real_inode(tu->path.dentry);
>
> -       if (tu->ref_ctr_offset)
> -               ret = uprobe_register_refctr(tu->inode, tu->offset,
> -                               tu->ref_ctr_offset, &tu->consumer);
> -       else
> -               ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
> -
> +       ret = uprobe_register(tu->inode, tu->offset, tu->ref_ctr_offset,
> +                             &tu->consumer);

doesn't fit under 100 characters? If it does, please keep as a single line.

>         if (ret)
>                 tu->inode = NULL;
>
> --
> 2.25.1.362.g51ebf55
>
>