On Sun, 27 Jun 2021 10:10:24 +0900
Tetsuo Handa [off-list ref] wrote:

On 2021/06/27 3:22, Steven Rostedt wrote:

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

  (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