[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZtQyxn9ZpxC12eFh@krava>
Date: Sun, 1 Sep 2024 11:24:22 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Jiri Olsa <olsajiri@...il.com>,
Andrii Nakryiko <andrii.nakryiko@...il.com>,
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, paulmck@...nel.org,
willy@...radead.org, surenb@...gle.com, akpm@...ux-foundation.org,
linux-mm@...ck.org
Subject: Re: [PATCH v4 4/8] uprobes: travers uprobe's consumer list
locklessly under SRCU protection
On Sat, Aug 31, 2024 at 07:25:44PM +0200, Oleg Nesterov wrote:
> On 08/30, Jiri Olsa wrote:
> >
> > with this change the probe will not get removed in the attached test,
> > it'll get 2 hits, without this change just 1 hit
>
> Thanks again for pointing out the subtle change in behaviour, but could
> you add more details for me? ;)
>
> I was going to read the test below today, but no. As I said many times
> I know nothing about bpf, I simply can't understand what this test-case
> actually do in kernel-space.
>
> According to git grep, the only in kernel user of UPROBE_HANDLER_REMOVE
> is uprobe_perf_func(), but if it returns UPROBE_HANDLER_REMOVE then
> consumer->filter == uprobe_perf_filter() should return false?
>
> So could you explay how/why exactly this changes affects your test-case?
>
>
> But perhaps it uses bpf_uprobe_multi_link_attach() and ->handler is
> uprobe_multi_link_handler() ? But uprobe_prog_run() returns zero if
> current->mm != link->task->mm.
>
> OTOH, otherwise it returns the error code from bpf_prog_run() and this looks
> confusing to me. I have no idea what prog->bpf_func(ctx, insnsi) can return
> in this case, but note the WARN(rc & ~UPROBE_HANDLER_MASK) in handler_chain...
>
> Hmm... looking at your test-case again,
>
> > +SEC("uprobe.multi//proc/self/exe:uprobe_multi_func_1")
> > +int uprobe(struct pt_regs *ctx)
> > +{
> > + test++;
> > + return 1;
> > +}
>
> So may be this (compiled to ebpf) is what prog->bpf_func() actually executes?
yep, that's correct, it goes like:
uprobe_multi_link_handler
uprobe_prog_run
{
err = bpf_prog_run - runs above bpf program and returns its return
value (1 - UPROBE_HANDLER_REMOVE)
return err;
}
> If yes, everything is clear. And this "proves" that the patch makes the current
> API less flexible, as I mentioned in my reply to Andrii.
>
> If I got it right, I'd suggest to add a comment into this code to explain
> that we return UPROBE_HANDLER_REMOVE after the 1st hit, for git-grep.
ok, I'll add comment with that
thanks,
jirka
Powered by blists - more mailing lists