Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Jiri Olsa <jolsa@redhat.com>,
	Eelco Chaudron <echaudro@redhat.com>,
	KP Singh <kpsingh@chromium.org>,
	netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach
Date: Wed, 23 Sep 2020 17:14:39 -0700	[thread overview]
Message-ID: <20200924001439.qitbu5tmzz55ck4z@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <160079991808.8301.6462172487971110332.stgit@toke.dk>

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 ?