Re: [PATCH v2 bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops

From: Song Liu <songliubraving@fb.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Song Liu <song@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"ast@kernel.org" <ast@kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"andrii@kernel.org" <andrii@kernel.org>,
	Kernel Team <Kernel-team@fb.com>,
	"jolsa@kernel.org" <jolsa@kernel.org>,
	"mhiramat@kernel.org" <mhiramat@kernel.org>
Subject: Re: [PATCH v2 bpf-next 1/5] ftrace: allow customized flags for ftrace_direct_multi ftrace_ops
Date: Thu, 14 Jul 2022 01:42:59 +0000	[thread overview]
Message-ID: <C2FCCC9B-5F7D-4BBF-8410-67EA79166909@fb.com> (raw)
In-Reply-To: <20220713203841.76d66245@rorschach.local.home>

> On Jul 13, 2022, at 5:38 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 14 Jul 2022 00:11:53 +0000
> Song Liu <songliubraving@fb.com> wrote:
> 
>>> That is, can we register a direct function with this function and pick a
>>> function with IPMODIFY already attached?  
>> 
>> Yes, if the direct function follows regs->ip, it works. 
>> 
>> For example, BPF trampoline with only fentry calls will just work with only this patch.
> 
> I replied with my thoughts on this to patch 3, but I disagree with this.
> 
> ftrace has no idea if the direct trampoline modifies the IP or not.
> ftrace must assume that it does, and the management should be done in
> the infrastructure.
> 
> As I replied to patch 3, here's my thoughts.
> 
> DIRECT is treated as though it changes the IP. If you register it to a
> function that has an IPMODIFY already set to it, it will call the
> ops->ops_func() asking if the ops can use SHARED_IPMODIFY (which means
> a DIRECT can be shared with IPMODIFY). If it can, then it returns true,
> and the SHARED_IPMODIFY is set *by ftrace*. The user of the ftrace APIs
> should not have to manage this. It should be managed by the ftrace
> infrastructure.

Hmm... I don't think this gonna work. 

First, two IPMODIFY ftrace ops cannot work together on the same kernel 
function. So there won't be a ops with both IPMODIFY and SHARE_IPMODIFY. 

non-direct ops without IPMODIFY can already share with IPMODIFY ops.
Only direct ops need SHARE_IPMODIFY flag, and it means "I can share with 
another ops with IPMODIFY". So there will be different flavors of 
direct ops:

  1. w/ IPMODIFY, w/o SHARE_IPMODIFY;
  2. w/o IPMODIFY, w/o SHARE_IPMODIFY;
  3. w/o IPMODIFY, w/ SHARE_IPMODIFY. 

#1 can never work on the same function with another IPMODIFY ops, and 
we don't plan to make it work. #2 does not work with another IPMODIFY 
ops. And #3 works with another IPMODIFY ops. 

The owner of the direct trampoline uses these flags to tell ftrace 
infrastructure the property of this trampoline. 

BPF trampolines with only fentry calls are #3 direct ops. BPF 
trampolines with fexit or fmod_ret calls will be #2 trampoline by 
default, but it is also possible to generate #3 trampoline for it.

BPF side will try to register #2 trampoline, If ftrace detects another 
IPMODIFY ops on the same function, it will let BPF trampoline know 
with -EAGAIN from register_ftrace_direct_multi(). Then, BPF side will 
regenerate a #3 trampoline and register it again. 

I know this somehow changes the policy with direct ops, but it is the
only way this can work, AFAICT. 

Does this make sense? Did I miss something?

Thanks,
Song