Re: [PATCHv3 bpf-next 2/2] selftests/bpf: Add uprobe multi consumers test

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Sep 24, 2024 at 07:44:50AM +0200, Andrii Nakryiko wrote:

SNIP

> > > + /
> > > + * uprobe return is tricky ;-)
> > > + *
> > > + * to trigger uretprobe consumer, the uretprobe needs to be installed,
> > > + * which means one of the 'return' uprobes was alive when probe was hit:
> > > + *
> > > + * idxs: 2/3 uprobe return in 'installed' mask
> > > + *
> > > + * in addition if 'after' state removes everything that was installed in
> > > + * 'before' state, then uprobe kernel object goes away and return uprobe
> > > + * is not installed and we won't hit it even if it's in 'after' state.
> > > + */
> > > + unsigned long had_uretprobes = before & 0b1100; // is uretprobe installed
> > > + unsigned long probe_preserved = before & after; // did uprobe go away
> > > +
> > > + if (had_uretprobes && probe_preserved && test_bit(idx, after))
> > > + val++;
> > > + fmt = "idx 2/3: uretprobe";
> > > + }
> >
> > Jiri, Andrii,
> >
> > This test case started failing since upstream got merged into bpf-next,
> > starting from commit https://git.kernel.org/bpf/bpf-next/c/440b65232829

thanks for the report

SNIP

> 
> Thanks for the mitigation! I think this is due to my recent RCU and
> refcounting changes to uprobes/uretprobes, which went through
> tip/perf/core initially. And now that tip and bpf-next trees
> converged, this condition:
> 
>   > unsigned long probe_preserved = before & after; // did uprobe go away
> 
> is no longer correct, and uretprobe can be activated if there was
> *any* uretprobe installed before.
> 
> So the test needs adjustment, but I don't think anything really broke.
> I don't remember exactly (and given the conferencing schedule and
> quite bad internet can't test quickly), but I think the condition
> should now be:
> 
> unsigned long probe_preserved = after & 0x1100;
> 
> (though we might want to also rename the variable to be a bit more
> meaningful now).
> 
> Anyways, I don't think this is critical and we can address this later.
> But if anyone is willing to send a fix, I'd appreciate it, of course!

I think we can remove that check completely.. I sent the patch, let's discuss there ;-)

thanks,
jirka