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
