linux-kernel - Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to 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: <20240730171733.GA10822@redhat.com>
Date: Tue, 30 Jul 2024 19:17:34 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: andrii@...nel.org, mhiramat@...nel.org, peterz@...radead.org,
	jolsa@...nel.org, rostedt@...dmis.org, linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to
 uprobe_unregister()

Thanks for looking at this!

On 07/30, Andrii Nakryiko wrote:
>
> 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.

No, I have nothing against.

To be honest, I don't really understand the value of this change, but
a) this is probably because I didn't see a separate patch(es) which
does this and b) assuming that it doesn't complicate the code too much
I won't argue in any case ;)

And in fact I had your proposed change in mind when I did these cleanups.
I think that they can even simplify this change, at least I hope they can
not complicate it.

> 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 :)

Well yes... may be this week.

I'd like to (try to) optimize/de-uglify register_for_each_vma() for
the multiple-consumers case. And, more importantly, the case when perf
does uprobe_register() + uprobe_apply().

But. All these changes will only touch the register_for_each_vma() paths,
so this is completely orthogonal to this series and your and/or Peter's
changes.

> 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!

Cough ;)

No, sorry, I can't. I lost my kernel.org account years ago (and this is
the second time this has happened!), but since I am a lazy dog I didn't
even bother to try to restore it.

> Acked-by: Andrii Nakryiko <andrii@...nel.org>

Thanks!

> > @@ -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.

Agreed, uprobe->consumers != NULL is not an error. But we don't return
this error code, just we need to ensure that "err == 0" means that
"delete_uprobe() was actually called".

> With my
> proposed change to refcounting schema this would be unnecessary,

Yes.

Oleg.