Re: [PATCH] tracepoint: Do not warn on EEXIST or ENOENT

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 27 Jun 2021 10:10:24 +0900
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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