Re: [PATCH v4 4/8] uprobes: travers uprobe's consumer list locklessly under SRCU protection

[Jiri Olsa]

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