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@xxxxxx>
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:
