On Sun, 27 Jun 2021 10:10:24 +0900 Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote: > On 2021/06/27 3:22, Steven Rostedt wrote: > >> If BPF is expected to register the same tracepoint with the same > >> callback and data more than once, then let's add a call to do that > >> without warning. Like I said, other callers expect the call to succeed > >> unless it's out of memory, which tends to cause other problems. > > > > If BPF is OK with registering the same probe more than once if user > > space expects it, we can add this patch, which allows the caller (in > > this case BPF) to not warn if the probe being registered is already > > registered, and keeps the idea that a probe registered twice is a bug > > for all other use cases. > > I think BPF will not register the same tracepoint with the same callback and > data more than once, for bpf(BPF_RAW_TRACEPOINT_OPEN) cleans the request up > by calling bpf_link_cleanup() and returns -EEXIST. But I think BPF relies on > tracepoint_add_func() returning -EEXIST without crashing the kernel. Which is the only user that does so, and what this patch addresses. > > That's because (before BPF) there's no place in the kernel that tries > > to register the same tracepoint multiple times, and was considered a > > bug if it happened, because there's no ref counters to deal with adding > > them multiple times. > > I see. But does that make sense? Since func_add() can fail with -ENOMEM, > all places (even before BPF) needs to be prepared for failures. Yes. -ENOMEM means that there's no resources to create a tracepoint. But if the tracepoint already exsits, that means the accounting for what tracepoints are running has been corrupted. > > > > > If the tracepoint is already registered (with the given function and > > data), then something likely went wrong. > > That can be prepared on the caller side of tracepoint_add_func() rather than > tracepoint_add_func() side. Not sure what you mean by that. > > > > >> (3) And tracepoint_add_func() is triggerable via request from userspace. > > > > Only via BPF correct? > > > > I'm not sure how it works, but can't BPF catch that it is registering > > the same tracepoint again? > > There is no chance to check whether some tracepoint is already registered, for > tracepoints_mutex is the only lock which gives us a chance to check whether > some tracepoint is already registered. > > Should bpf() syscall hold a global lock (like tracepoints_mutex) which will serialize > the entire code in order to check whether some tracepoint is already registered? > That might severely damage concurrency. I think that the patch I posted handles what you want. For BPF it returns without warning, but for all other cases, it warns. Does it fix your issue? -- Steve
