Re: [PATCH bpf-next] bpf: add bpf_link_new_file that doesn't install FD

[Daniel Borkmann <daniel@iogearbox.net>]

On 3/10/20 12:10 AM, Andrii Nakryiko wrote:
> Add bpf_link_new_file() API for cases when we need to ensure anon_inode is
> successfully created before we proceed with expensive BPF program attachment
> procedure, which will require equally (if not more so) expensive and
> potentially failing compensation detachment procedure just because anon_inode
> creation failed. This API allows to simplify code by ensuring first that
> anon_inode is created and after BPF program is attached proceed with
> fd_install() that can't fail.
> 
> After anon_inode file is created, link can't be just kfree()'d anymore,
> because its destruction will be performed by deferred file_operations->release
> call. For this, bpf_link API required specifying two separate operations:
> release() and dealloc(), former performing detachment only, while the latter
> frees memory used by bpf_link itself. dealloc() needs to be specified, because
> struct bpf_link is frequently embedded into link type-specific container
> struct (e.g., struct bpf_raw_tp_link), so bpf_link itself doesn't know how to
> properly free the memory. In case when anon_inode file was successfully
> created, but subsequent BPF attachment failed, bpf_link needs to be marked as
> "defunct", so that file's release() callback will perform only memory
> deallocation, but no detachment.
> 
> Convert raw tracepoint and tracing attachment to new API and eliminate
> detachment from error handling path.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Applied, but ...

[...]
> @@ -2337,20 +2374,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>   	}
>   	bpf_link_init(&link->link, &bpf_tracing_link_lops, prog);
>   
> -	err = bpf_trampoline_link_prog(prog);
> -	if (err)
> -		goto out_free_link;
> +	link_file = bpf_link_new_file(&link->link, &link_fd);
> +	if (IS_ERR(link_file)) {
> +		kfree(link);
> +		err = PTR_ERR(link_file);
> +		goto out_put_prog;
> +	}
>   
> -	link_fd = bpf_link_new_fd(&link->link);
> -	if (link_fd < 0) {
> -		WARN_ON_ONCE(bpf_trampoline_unlink_prog(prog));
> -		err = link_fd;
> -		goto out_free_link;
> +	err = bpf_trampoline_link_prog(prog);
> +	if (err) {
> +		bpf_link_defunct(&link->link);
> +		fput(link_file);
> +		put_unused_fd(link_fd);

Given the tear-down in error case requires 3 manual steps here, I think this begs
for a small helper.

> +		goto out_put_prog;
>   	}
> +
> +	fd_install(link_fd, link_file);
>   	return link_fd;
>   
[...]
> @@ -2431,28 +2481,32 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>   		goto out_put_prog;
>   	}
>   
[...]
>   
> -	link_fd = bpf_link_new_fd(&raw_tp->link);
> -	if (link_fd < 0) {
> -		bpf_probe_unregister(raw_tp->btp, prog);
> -		err = link_fd;
> -		goto out_free_tp;
> +	err = bpf_probe_register(link->btp, prog);
> +	if (err) {
> +		bpf_link_defunct(&link->link);
> +		fput(link_file);
> +		put_unused_fd(link_fd);

Especially since you need it in multiple places; please follow-up.

> +		goto out_put_btp;
>   	}
> +
> +	fd_install(link_fd, link_file);
>   	return link_fd;
>   
> -out_free_tp:
> -	kfree(raw_tp);
>   out_put_btp:
>   	bpf_put_raw_tracepoint(btp);
>   out_put_prog:
>