Message-ID: <20171113010203.at4szpaosiw6xp7q@ast-mbp>
Date:   Mon, 13 Nov 2017 09:02:05 +0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     yupeng0921@...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

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