Re: [PATCH bpf-next v7 3/5] bpf, x86: Attach a cookie to fentry/fexit/fmod_ret/lsm.

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sat, May 7, 2022 at 8:21 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> Pass a cookie along with BPF_LINK_CREATE requests.
>
> Add a bpf_cookie field to struct bpf_tracing_link to attach a cookie.
> The 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@fb.com>
> ---
>  arch/x86/net/bpf_jit_comp.c    | 12 ++++++++++--
>  include/linux/bpf.h            |  1 +
>  include/uapi/linux/bpf.h       |  9 +++++++++
>  kernel/bpf/bpf_lsm.c           | 17 +++++++++++++++++
>  kernel/bpf/syscall.c           | 12 ++++++++----
>  kernel/bpf/trampoline.c        |  7 +++++--
>  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  9 +++++++++
>  8 files changed, 76 insertions(+), 8 deletions(-)
>

LGTM with a suggestion for some follow up clean up.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index bf4576a6938c..52a5eba2d5e8 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1764,13 +1764,21 @@ 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) {

It would probably be nicer to put cookie field into struct
bpf_tramp_link instead so that the JIT compiler doesn't have to do
this special handling. It also makes sense that struct bpf_trampoline
*trampoline is moved into struct bpf_tramp_link itself (given
trampoline is always there for bpf_tramp_link).

> +               struct bpf_tracing_link *tr_link =
> +                       container_of(l, struct bpf_tracing_link, link);
> +
> +               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.
>          *

[...]