On Tue, Sep 22, 2020 at 08:38:38PM +0200, Toke Høiland-Jørgensen wrote:
> @@ -746,7 +748,9 @@ struct bpf_prog_aux {
> u32 max_rdonly_access;
> u32 max_rdwr_access;
> const struct bpf_ctx_arg_aux *ctx_arg_info;
> - struct bpf_prog *linked_prog;
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
> + struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
> + struct bpf_prog *tgt_prog;
> + struct bpf_trampoline *tgt_trampoline;
> bool verifier_zext; /* Zero extensions has been inserted by verifier. */
> bool offload_requested;
> bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
...
> struct bpf_tracing_link {
> struct bpf_link link;
> enum bpf_attach_type attach_type;
> + struct bpf_trampoline *trampoline;
> + struct bpf_prog *tgt_prog;
imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
> };
>
> static void bpf_tracing_link_release(struct bpf_link *link)
> {
> - WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog));
> + struct bpf_tracing_link *tr_link =
> + container_of(link, struct bpf_tracing_link, link);
> +
> + WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog,
> + tr_link->trampoline));
> +
> + bpf_trampoline_put(tr_link->trampoline);
> +
> + if (tr_link->tgt_prog)
> + bpf_prog_put(tr_link->tgt_prog);
I had to scratch my head quite a bit before I understood this NULL check.
Could you add a comment saying that tr_link->tgt_prog can be NULL
when trampoline is for kernel function ?
