On Fri, Aug 30, 2024 at 7:33 AM Oleg Nesterov <oleg@xxxxxxxxxx> 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 > > I don't understand the code in tools/...bpf../ at all, can't comment, > > > but I'm not sure it's a big problem, because seems like that's not the > > intended way the removal should be used anyway, as explained by Oleg [1] > > It seems that I confused you again ;) > > No, I think you found a problem. UPROBE_HANDLER_REMOVE can be lost if > uc->filter == NULL of if it returns true. See another reply I sent a > minute ago. > For better or worse, but I think there is (or has to be) and implicit contract that if uprobe (or uretprobe for that matter as well, but that's a separate issue) handler can return UPROBE_HANDLER_REMOVE, then it *has to* also provide filter. If it doesn't provide filter callback, it doesn't care about PID filtering and thus can't and shouldn't cause unregistration. In ideal world, we wouldn't need handler to do the filtering, and instead generic uprobe/uretprobe code would just call uc->filter to know whether to trigger consumer or not. Unfortunately, that's a lot of overhead due to indirect function call, especially with retpolines and stuff like that. So I think it's reasonable to have an (implicit, yeah...) contract that whoever cares about UPROBE_HANDLER_REMOVE has to provide filter, they go together. Jiri, the fact that uprobe/uretprobe can cause detachment by returning 1 is a bug, we should not allow that. But that's a separate issue which we can fix in bpf-next tree. Please send a patch. > I think the fix is simple, plus we need to cleanup this logic anyway, > I'll try to send some code on Monday. Can we please let me land these patches first? It's been a while. I don't think anything is really broken with the logic. > > Oleg. >
