Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 5:35 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> Kill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and
> move the possibly final put_uprobe() from delete_uprobe() to its only
> caller, uprobe_unregister().
>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
>  kernel/events/uprobes.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>

LGTM as well.

BTW, do you have anything against me changing refcounting so that
uprobes_tree itself doesn't hold a refcount, and all the refcounting
is done based on consumers holding implicit refcount and whatever
temporary get/put uprobe is necessary for runtime uprobe/uretprobe
functioning.

I can do that with a simple refcount_t and refcount_inc_not_zero(), no
fancy custom refcounting. This schema makes most sense to me, it will
also simplify registration by avoiding that annoying
is_uprobe_active() check + retry. Thoughts?

All that would be additional on top of your change. BTW, do you plan
any more clean ups like this? It's a bit of a moving target because of
your refactoring, so I'd appreciate some stability to build upon :)

Also, can you please push this and your previous patch set into some
branch somewhere I can pull from, preferably based on the latest
linux-trace's probes/for-next? Thanks!

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index c06e1a5f1783..f88b7ff20587 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe)
>         rb_erase(&uprobe->rb_node, &uprobes_tree);
>         write_unlock(&uprobes_treelock);
>         RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */
> -       put_uprobe(uprobe);
>  }
>
>  struct map_info {
> @@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
>         int err;
>
> -       get_uprobe(uprobe);
>         down_write(&uprobe->register_rwsem);
>         if (WARN_ON(!consumer_del(uprobe, uc)))
>                 err = -ENOENT;
> @@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>                 err = register_for_each_vma(uprobe, NULL);
>
>         /* TODO : cant unregister? schedule a worker thread */
> -       if (!err && !uprobe->consumers)
> -               delete_uprobe(uprobe);
> +       if (!err) {
> +               if (!uprobe->consumers)
> +                       delete_uprobe(uprobe);
> +               else
> +                       err = -EBUSY;

This bit is weird because really it's not an error... but this makes
this change simpler and moves put_uprobe outside of rwsem. With my
proposed change to refcounting schema this would be unnecessary, but
let's see what you think.

> +       }
>         up_write(&uprobe->register_rwsem);
> -       put_uprobe(uprobe);
> +
> +       if (!err)
> +               put_uprobe(uprobe);
>  }
>  EXPORT_SYMBOL_GPL(uprobe_unregister);
>
> --
> 2.25.1.362.g51ebf55
>