On Tue, Apr 12, 2022 at 9:56 AM Kui-Feng Lee <kuifeng@xxxxxx> wrote:
>
> Add a bpf_cookie field to struct bpf_tracing_link to attach a cookie
> to a link of a trace hook. A cookie of a bpf_tracing_link is
> available by calling bpf_get_attach_cookie when running the BPF
> program of the attached link.
>
> The value of a cookie will be set at bpf_tramp_run_ctx by the
> trampoline of the link.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxx>
> ---
> arch/x86/net/bpf_jit_comp.c | 11 +++++++++--
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 8 ++++++++
> kernel/bpf/syscall.c | 27 +++++++++++++++++++++++----
> kernel/trace/bpf_trace.c | 17 +++++++++++++++++
> tools/include/uapi/linux/bpf.h | 7 +++++++
> 6 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 0f521be68f7b..18d02dcd810a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1764,13 +1764,20 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
> struct bpf_tramp_link *l, int stack_size,
> bool save_ret)
> {
> + u64 cookie = 0;
> u8 *prog = *pprog;
> u8 *jmp_insn;
> int ctx_cookie_off = offsetof(struct bpf_tramp_run_ctx, bpf_cookie);
> struct bpf_prog *p = l->link.prog;
>
> - /* mov rdi, 0 */
> - emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> + if (l->link.type == BPF_LINK_TYPE_TRACING) {
> + struct bpf_tracing_link *tr_link =
> + container_of(l, struct bpf_tracing_link, link);
empty line between variable declaration block and statement
> + cookie = tr_link->cookie;
> + }
> +
> + /* mov rdi, cookie */
> + emit_mov_imm64(&prog, BPF_REG_1, (long) cookie >> 32, (u32) (long) cookie);
>
> /* Prepare struct bpf_tramp_run_ctx.
> *
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d87df049e6b1..49db9c065701 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1062,6 +1062,7 @@ struct bpf_tracing_link {
> enum bpf_attach_type attach_type;
> struct bpf_trampoline *trampoline;
> struct bpf_prog *tgt_prog;
> + u64 cookie;
> };
>
> struct bpf_link_primer {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a4f557338af7..5e901e6d17ea 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1436,6 +1436,7 @@ union bpf_attr {
> struct { /* anonymous struct used by BPF_RAW_TRACEPOINT_OPEN command */
> __u64 name;
> __u32 prog_fd;
> + __u64 bpf_cookie;
I thought we are not adding bpf_cookie through RAW_TRACEPOINT_OPEN?
> } raw_tracepoint;
>
> struct { /* anonymous struct for BPF_BTF_LOAD */
> @@ -1490,6 +1491,13 @@ union bpf_attr {
> __aligned_u64 addrs;
> __aligned_u64 cookies;
> } kprobe_multi;
> + struct {
> + /* black box user-provided value passed through
> + * to BPF program at the execution time and
> + * accessible through bpf_get_attach_cookie() BPF helper
> + */
> + __u64 bpf_cookie;
> + } tracing;
> };
> } link_create;
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 56e69a582b21..53d4da5c76b5 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2695,9 +2695,10 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
> .fill_link_info = bpf_tracing_link_fill_link_info,
> };
>
> -static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> - int tgt_prog_fd,
> - u32 btf_id)
> +static int bpf_tracing_prog_attach_cookie(struct bpf_prog *prog,
> + int tgt_prog_fd,
> + u32 btf_id,
> + u64 bpf_cookie)
let's keep it as bpf_tracing_prog_attach and just update all the call sites
> {
> struct bpf_link_primer link_primer;
> struct bpf_prog *tgt_prog = NULL;
> @@ -2762,6 +2763,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> bpf_link_init(&link->link.link, BPF_LINK_TYPE_TRACING,
> &bpf_tracing_link_lops, prog);
> link->attach_type = prog->expected_attach_type;
> + link->cookie = bpf_cookie;
>
> mutex_lock(&prog->aux->dst_mutex);
>
> @@ -2871,6 +2873,13 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> return err;
> }
>
> +static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> + int tgt_prog_fd,
> + u32 btf_id)
> +{
> + return bpf_tracing_prog_attach_cookie(prog, tgt_prog_fd, btf_id, 0);
> +}
> +
why this wrapper, just add that 0 for all existing
bpf_tracing_prog_attach() invocations
> struct bpf_raw_tp_link {
> struct bpf_link link;
> struct bpf_raw_event_map *btp;
> @@ -3023,7 +3032,7 @@ static int bpf_perf_link_attach(const union bpf_attr *attr, struct bpf_prog *pro
> }
> #endif /* CONFIG_PERF_EVENTS */
>
> -#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
> +#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.bpf_cookie
>
> static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> {
> @@ -3187,6 +3196,10 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
> return BPF_PROG_TYPE_SK_LOOKUP;
> case BPF_XDP:
> return BPF_PROG_TYPE_XDP;
> + case BPF_TRACE_FENTRY:
> + case BPF_TRACE_FEXIT:
> + case BPF_MODIFY_RETURN:
> + return BPF_PROG_TYPE_TRACING;
can you please split the part that adds ability to create
fentry/fexit/etc links through LINK_CREATE into a separate commit,
separate from wiring the bpf_cookie? It's two logically distinct
changes.
> default:
> return BPF_PROG_TYPE_UNSPEC;
> }
[...]
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index a4f557338af7..a5ee57f09e04 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1490,6 +1490,13 @@ union bpf_attr {
> __aligned_u64 addrs;
> __aligned_u64 cookies;
> } kprobe_multi;
> + struct {
> + /* black box user-provided value passed through
> + * to BPF program at the execution time and
> + * accessible through bpf_get_attach_cookie() BPF helper
> + */
> + __u64 bpf_cookie;
for kprobe_multi we used "cookies", so let's do "cookie" here? We'll
have to keep perf_event.bpf_cookie, of course.
> + } tracing;
> };
> } link_create;
>
> --
> 2.30.2
>
