linux-kernel - Re: [PATCH 5/8] uprobes: travers uprobe's consumer list locklessly under SRCU protection

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: <CAEf4BzZ=W3JLfpYcxEevMGS4whXQ2-nn5ezA+p3zV_WhiGG4iQ@mail.gmail.com>
Date: Mon, 5 Aug 2024 10:31:55 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org, 
	peterz@...radead.org, rostedt@...dmis.org, mhiramat@...nel.org, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, jolsa@...nel.org, 
	paulmck@...nel.org
Subject: Re: [PATCH 5/8] uprobes: travers uprobe's consumer list locklessly
 under SRCU protection

On Mon, Aug 5, 2024 at 8:59 AM Oleg Nesterov <oleg@...hat.com> wrote:
>
> On 07/31, Andrii Nakryiko wrote:
> >
> > @@ -1120,17 +1098,19 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> >       int err;
> >
> >       down_write(&uprobe->register_rwsem);
> > -     if (WARN_ON(!consumer_del(uprobe, uc))) {
> > -             err = -ENOENT;
>
> OK, I agree, this should never happen.
>
> But if you remove this check, then
>
> >  int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool add)
> >  {
> >       struct uprobe_consumer *con;
> > -     int ret = -ENOENT;
> > +     int ret = -ENOENT, srcu_idx;
> >
> >       down_write(&uprobe->register_rwsem);
> > -     for (con = uprobe->consumers; con && con != uc ; con = con->next)
> > -             ;
> > -     if (con)
> > -             ret = register_for_each_vma(uprobe, add ? uc : NULL);
> > +
> > +     srcu_idx = srcu_read_lock(&uprobes_srcu);
> > +     list_for_each_entry_srcu(con, &uprobe->consumers, cons_node,
> > +                              srcu_read_lock_held(&uprobes_srcu)) {
> > +             if (con == uc) {
> > +                     ret = register_for_each_vma(uprobe, add ? uc : NULL);
> > +                     break;
> > +             }
> > +     }
>
> we can probably remove the similar check above?
>
> I mean, why do we need the list_for_each_entry_srcu() above? Is it possible
> that uprobe_apply(uprobe, uc) is called when "uc" is not on the ->consumers
> list?

Tbh, I just don't completely understand how (and why) uprobe_apply()
is used from kernel/trace/trace_uprobe.c, so I wanted to preserve the
logic exactly. I still don't see when this consumer is added before
uprobe_apply()... Exposing uprobe_apply() seems like a huge API
violation to me and I'd rather get rid of its users. But one step at a
time.


>
> At first glance I see no problems in this patch... but you know, my eyes are
> already blurring, I'll continue tomorrow and read this patch again.
>
> Oleg.
>