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
| ||
|
Date: Mon, 13 Nov 2017 00:06:17 -0800 From: peng yu <yupeng0921@...il.com> To: Alexei Starovoitov <alexei.starovoitov@...il.com> Cc: linux-kernel@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, rostedt@...dmis.org, mingo@...hat.com, "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org Subject: Re: [ftrace-bpf 1/5] add BPF_PROG_TYPE_FTRACE to bpf > 1. anything bpf related has to go via net-next tree. I found there is a net-next git repo: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git I will use this repo for the further bpf-ftrace patch set. > 2. > this obviously breaks ABI. New types can only be added to the end. Sure, I will add the new type at the end. > 3. > this won't even compile, since ftrace_regs is only added in the patch 4. It could compile, as the ftrace_regs related code is inside the "#ifdef FTRACE_BPF_FILTER" macro, if this macro is not defined, no ftrace_regs related code would be compiled. > Since bpf program will see ftrace_regs as an input it becomes > abi, so has to be defined in uapi/linux/bpf_ftrace.h or similar. > We need to think through how to make it generic across archs > instead of defining ftrace_regs for each arch. I'm not sure whether I'm fully understand your meaning. Like kprobe, the ftrace-bpf need to get a function's parameters and check them. So it won't be abi stable, and it should depend on architecture implement. I can create a header file like uapi/linux/bpf_ftrace.h, but I noticed that kprobe doesn't have such a header file, if I'm wrong, please let me know. And about make it generic across archs, I know kprobe use pt_regs as parameter, the pt_regs is defined on each arch, so I can't see how bpf-ftrace can get a generic interface across archs if it need to check function's parameters. If I misunderstand anything, please let me know. > 4. > the patch 2/3 takes an approach of passing FD integer value in text form > to the kernel. That approach was discussed years ago and rejected. > It has to use binary interface like perf_event + ioctl. > See RFC patches where we're extending perf_event_open syscall to > support binary access to kprobe/uprobe. > imo binary interface to ftrace is pre-requisite to ftrace+bpf work. > We've had too many issues with text based kprobe api to repeat > the same mistake here. I notice the kprobe-bpf prog is set through the PERF_EVENT_IOC_SET_BPF ioctl, I may try to see whether I can reuse this interface, or if it is not suitable, I will try to define a new binary interface. > 5. > patch 4 hacks save_mcount_regs asm to pass ctx pointer in %rcx > whereas it's only used in ftrace_graph_caller which doesn't seem right. > It points out to another issue that such ftrace+bpf integration > is only done for ftrace_graph_caller without extensibility in mind. > If we do ftrace+bpf I'd rather see generic framework that applies > to all of ftrace instead of single feature of it. It is a hard problem. The ftrace framework has lots of tracers, function tracer and function graph tracer use the 'gcc -pg' directly, other tracers use tracepoint, I should spend more time to find a suitable solution. > 6. > copyright line copy-pasted incorrectly. I will fix it. Summary: The question 1,2 and 6 are easy to fix. About question 4, I need to do more research, it shouldn't be very hard. About question 3 and 5, both 2017-11-12 17:02 GMT-08:00 Alexei Starovoitov <alexei.starovoitov@...il.com>: > On Sun, Nov 12, 2017 at 07:28:24AM +0000, yupeng0921@...il.com wrote: >> Add a new type BPF_PROG_TYPE_FTRACE to bpf, let bpf can be attached to >> ftrace. Ftrace pass the function parameters to bpf prog, bpf prog >> return 1 or 0 to indicate whether ftrace can trace this function. The >> major propose is provide an accurate way to trigger function graph >> trace. Changes in code: >> 1. add FTRACE_BPF_FILTER in kernel/trace/Kconfig. Let ftrace pass >> function parameter to bpf need to modify architecture dependent code, >> so this feature will only be enabled only when it is enabled in >> Kconfig and the architecture support this feature. If an architecture >> support this feature, it should define a macro whose name is >> FTRACE_BPF_FILTER, e.g.: >> So other code in kernel can check whether the macro FTRACE_BPF_FILTER >> is defined to know whether this feature is really enabled. >> 2. add BPF_PROG_TYPE_FTRACE in bpf_prog_type >> 3. check kernel version when load BPF_PROG_TYPE_FTRACE bpf prog >> 4. define ftrace_prog_func_proto, the prog input is a struct >> ftrace_regs type pointer, it is similar as pt_regs in kprobe, it >> is an architecture dependent code, if an architecture doens't define >> FTRACE_BPF_FILTER, use a fake ftrace_prog_func_proto. >> 5. add BPF_PROG_TYPE in bpf_types.h >> >> Signed-off-by: yupeng0921@...il.com > > In general I like the bigger concept of adding bpf filtering to ftrace, > but there are a lot of fundamental issues with this patch set. > > 1. anything bpf related has to go via net-next tree. > >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -118,6 +118,7 @@ enum bpf_prog_type { >> BPF_PROG_TYPE_UNSPEC, >> BPF_PROG_TYPE_SOCKET_FILTER, >> BPF_PROG_TYPE_KPROBE, >> + BPF_PROG_TYPE_FTRACE, >> BPF_PROG_TYPE_SCHED_CLS, > > 2. > this obviously breaks ABI. New types can only be added to the end. > >> +static bool ftrace_prog_is_valid_access(int off, int size, >> + enum bpf_access_type type, >> + struct bpf_insn_access_aux *info) >> +{ >> + if (off < 0 || off >= sizeof(struct ftrace_regs)) >> + return false; > > 3. > this won't even compile, since ftrace_regs is only added in the patch 4. > > Since bpf program will see ftrace_regs as an input it becomes > abi, so has to be defined in uapi/linux/bpf_ftrace.h or similar. > We need to think through how to make it generic across archs > instead of defining ftrace_regs for each arch. > > 4. > the patch 2/3 takes an approach of passing FD integer value in text form > to the kernel. That approach was discussed years ago and rejected. > It has to use binary interface like perf_event + ioctl. > See RFC patches where we're extending perf_event_open syscall to > support binary access to kprobe/uprobe. > imo binary interface to ftrace is pre-requisite to ftrace+bpf work. > We've had too many issues with text based kprobe api to repeat > the same mistake here. > > 5. > patch 4 hacks save_mcount_regs asm to pass ctx pointer in %rcx > whereas it's only used in ftrace_graph_caller which doesn't seem right. > It points out to another issue that such ftrace+bpf integration > is only done for ftrace_graph_caller without extensibility in mind. > If we do ftrace+bpf I'd rather see generic framework that applies > to all of ftrace instead of single feature of it. > > 6. > copyright line copy-pasted incorrectly. >
Powered by blists - more mailing lists