On Mon, Sep 06, 2024 at 18:43:00AM +0800, Jiri Olsa wrote: SNIP
> > > --------------------------------------------------------------------------- > > > Now lets return to BPF and this particular problem. I won't really argue > > > with this patch, but > > > > > > - Please change the subject and update the changelog, > > > "Fixes: c1ae5c75e103" and the whole reasoning is misleading > > > and wrong, IMO. > > > > > > - This patch won't fix all problems because uprobe_perf_filter() > > > filters by mm, not by pid. The changelog/patch assumes that it > > > is a "PID filter", but it is not. > > > > > > See > > > https://lore.kernel.org/linux-trace-kernel/20240825224018.gd3...@redhat.com/ > > > If the traced process does clone(CLONE_VM), bpftrace will hit the > > > similar problem, with uprobe or uretprobe. > > > > > > - So I still think that the "right" fix should change the > > > bpf_prog_run_array_uprobe() paths somehow, but I know nothing > > > about bpf. > > > > I agree that this patch does not address the issue correctly. > > The PID filter should be implemented within bpf_prog_run_array_uprobe, > > or alternatively, bpf_prog_run_array_uprobe should be called after > > perf_tp_event_match to reuse the filtering mechanism provided by perf. > > > > Also, uretprobe may need UPROBE_HANDLER_REMOVE, similar to uprobe. > > > > For now, please forget the original patch as we need a new solution ;) > > hi, > any chance we could go with your fix until we find better solution? > > it's simple and it fixes most of the cases for return uprobe pid filter > for events with bpf programs.. I know during the discussion we found > that standard perf record path won't work if there's bpf program > attached on the same event, but I think that likely it needs more > changes and also it's not a common use case > > would you consider sending another version addressing Oleg's points > for changelog above? My pleasure, I'll resend the updated patch in a new thread. Based on previous discussions, `uprobe_perf_filter` acts as a preliminary filter that removes breakpoints when they are no longer needed. More complex filtering mechanisms related to perf are implemented in perf-specific paths. >From my understanding, the original patch attempted to partially implement UPROBE_HANDLER_REMOVE (since it didn't actually remove the breakpoint but only prevented it from entering the BPF-related code). I'm trying to provide a complete implementation, i.e., removing the breakpoint when `uprobe_perf_filter` returns false, similar to how uprobe functions. However, this would require merging the following functions, because they will almost be the same: uprobe_perf_func / uretprobe_perf_func uprobe_dispatcher / uretprobe_dispatcher handler_chain / handle_uretprobe_chain I'm not sure why the paths of uprobe and uretprobe are entirely different. I suspect that uretprobe might have been implemented later than uprobe and was only partially implemented. Oleg, do you have more insights on this? In your opinion, does uretprobe need UPROBE_HANDLER_REMOVE? If so, is it a good idea to merge the paths of uprobe and uretprobe? Regardless, if we only need a temporary and incomplete fix, I will modify only the commit message according to Oleg's suggestions and resend it. I'm aware that using `uprobe_perf_filter` in `uretprobe_perf_func` is not the solution for BPF filtering. I'm just trying to alleviate the issue in some simple cases. Sorry for the late reply as I spent a long time looking at the details discussed above. Thanks,
