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

From: Steven Rostedt <rostedt@goodmis.org>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Ingo Molnar <mingo@kernel.org>, Robert Richter <rric@kernel.org>,
	Gabriel Krisman Bertazi <krisman@collabora.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	linux-kernel@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	netdev <netdev@vger.kernel.org>,
	bpf@vger.kernel.org
Subject: Re: [PATCH] tracepoint: Do not warn on EEXIST or ENOENT
Date: Sat, 26 Jun 2021 22:52:33 -0400	[thread overview]
Message-ID: <20210626225233.2baae8be@rorschach.local.home> (raw)
In-Reply-To: <fc5d0f90-502d-b217-0ad6-0d17cae12ff7@i-love.sakura.ne.jp>

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