On Sun, Nov 12, 2017 at 07:28:24AM +0000, yupeng0...@gmail.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: yupeng0...@gmail.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.
