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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <C2FCCC9B-5F7D-4BBF-8410-67EA79166909@fb.com>
Date:   Thu, 14 Jul 2022 01:42:59 +0000
From:   Song Liu <songliubraving@...com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     Song Liu <song@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "andrii@...nel.org" <andrii@...nel.org>,
        Kernel Team <Kernel-team@...com>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "mhiramat@...nel.org" <mhiramat@...nel.org>
Subject: Re: [PATCH v2 bpf-next 1/5] ftrace: allow customized flags for
 ftrace_direct_multi ftrace_ops

> On Jul 13, 2022, at 5:38 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> On Thu, 14 Jul 2022 00:11:53 +0000
> Song Liu <songliubraving@...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