[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240806105411.GB20881@redhat.com>
Date: Tue, 6 Aug 2024 12:54:11 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.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 08/05, Andrii Nakryiko wrote:
>
> On Mon, Aug 5, 2024 at 8:59 AM Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > > 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.
Agreed. Unlike uprobe_unregister(), uprobe_apply() doesn't WARN() or
even explains this check, lets preserve the current logic for now.
And just in case... I am not sure too that the con == NULL case is not
possible with the current code. The recent discussions forced me to recall
some bits in uprobe.c, but not in trace_uprobe.c ;)
Oleg.
Powered by blists - more mailing lists