-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ebpf: replace the existing program on update #352
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
extraction: | ||
cpp: | ||
configure: | ||
command: | ||
- ./autogen.sh | ||
- ./configure |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -293,15 +293,169 @@ bpf_program_complete_dev (struct bpf_program *program, libcrun_error_t *err arg_ | |
return program; | ||
} | ||
|
||
static int | ||
read_all_progs (int dirfd, uint32_t **progs_out, size_t *n_progs_out, libcrun_error_t *err) | ||
{ | ||
cleanup_free uint32_t *progs = NULL; | ||
union bpf_attr attr; | ||
int ret; | ||
size_t cur_size; | ||
|
||
/* The kernel 5.7 has a hard limit of 64, let's be safe if the limit | ||
will be increased in future and attempt up to 4096. */ | ||
for (cur_size = 64; cur_size <= 4096; cur_size *= 2) | ||
{ | ||
progs = xrealloc (progs, sizeof (uint32_t) * cur_size); | ||
|
||
memset (&attr, 0, sizeof (attr)); | ||
attr.query.target_fd = dirfd; | ||
attr.query.attach_type = BPF_CGROUP_DEVICE; | ||
attr.query.prog_cnt = cur_size; | ||
attr.query.prog_ids = (uint64_t) progs; | ||
|
||
ret = bpf (BPF_PROG_QUERY, &attr, sizeof (attr)); | ||
} | ||
while (ret < 0 && errno == ENOSPC); | ||
|
||
if (UNLIKELY (ret < 0)) | ||
return crun_make_error (err, errno, "bpf query"); | ||
|
||
*progs_out = progs; | ||
progs = NULL; | ||
*n_progs_out = attr.query.prog_cnt; | ||
return 0; | ||
} | ||
|
||
static int | ||
remove_all_progs (int dirfd, uint32_t *progs, size_t n_progs, libcrun_error_t *err) | ||
{ | ||
union bpf_attr attr; | ||
size_t i; | ||
|
||
for (i = 0; i < n_progs; i++) | ||
{ | ||
cleanup_close int fd = -1; | ||
int ret; | ||
|
||
memset (&attr, 0, sizeof (attr)); | ||
attr.prog_id = progs[i]; | ||
fd = bpf (BPF_PROG_GET_FD_BY_ID, &attr, sizeof (attr)); | ||
if (UNLIKELY (fd < 0)) | ||
{ | ||
/* Already removed. Nothing to do. */ | ||
if (errno == ENOENT) | ||
continue; | ||
|
||
return crun_make_error (err, errno, "cannot open existing eBPF program"); | ||
} | ||
|
||
memset (&attr, 0, sizeof (attr)); | ||
attr.attach_type = BPF_CGROUP_DEVICE; | ||
attr.target_fd = dirfd; | ||
attr.attach_bpf_fd = fd; | ||
|
||
ret = bpf (BPF_PROG_DETACH, &attr, sizeof (attr)); | ||
if (UNLIKELY (ret < 0)) | ||
{ | ||
/* Already removed. Nothing to do. */ | ||
if (errno == ENOENT) | ||
continue; | ||
|
||
return crun_make_error (err, errno, "cannot detach eBPF program"); | ||
} | ||
} | ||
return 0; | ||
} | ||
|
||
static int | ||
ebpf_attach_program (int fd, int dirfd, libcrun_error_t *err) | ||
{ | ||
#ifdef BPF_F_REPLACE | ||
bool skip_replace = false; | ||
#endif | ||
const int MAX_ATTEMPTS = 20; | ||
int attempt; | ||
|
||
for (attempt = 0;; attempt++) | ||
{ | ||
cleanup_free uint32_t *progs = NULL; | ||
cleanup_close int replacefd = -1; | ||
union bpf_attr attr; | ||
size_t n_progs = 0; | ||
int ret; | ||
|
||
ret = read_all_progs (dirfd, &progs, &n_progs, err); | ||
if (UNLIKELY (ret < 0)) | ||
return ret; | ||
|
||
#ifdef BPF_F_REPLACE | ||
/* There is just one program installed, let's attempt an atomic replace if supported. */ | ||
if (!skip_replace && n_progs == 1) | ||
{ | ||
memset (&attr, 0, sizeof (attr)); | ||
attr.prog_id = progs[0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic (replacing whatever program is attached) and the corresponding limitations (not being able to replace a program if there is more than one) seems a bit fishy. I think a much better solution would be to either pin the program, or to store the id and load-time into some state file (which I assume To be fair, it's unlikely anybody else will be attaching |
||
replacefd = bpf (BPF_PROG_GET_FD_BY_ID, &attr, sizeof (attr)); | ||
if (UNLIKELY (replacefd < 0)) | ||
{ | ||
if (errno == ENOENT && attempt < MAX_ATTEMPTS) | ||
{ | ||
/* Another update might have raced and updated, try again. */ | ||
continue; | ||
} | ||
return crun_make_error (err, errno, "cannot open existing eBPF program"); | ||
} | ||
} | ||
#endif | ||
|
||
memset (&attr, 0, sizeof (attr)); | ||
attr.attach_type = BPF_CGROUP_DEVICE; | ||
attr.target_fd = dirfd; | ||
attr.attach_bpf_fd = fd; | ||
attr.attach_flags = BPF_F_ALLOW_MULTI; | ||
#ifdef BPF_F_REPLACE | ||
if (replacefd >= 0) | ||
{ | ||
attr.attach_flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; | ||
attr.replace_bpf_fd = replacefd; | ||
} | ||
#endif | ||
|
||
ret = bpf (BPF_PROG_ATTACH, &attr, sizeof (attr)); | ||
if (UNLIKELY (ret < 0)) | ||
{ | ||
if (errno == ENOENT && replacefd >= 0 && attempt < MAX_ATTEMPTS) | ||
{ | ||
/* Another update might have already updated the cgroup, try again. */ | ||
continue; | ||
} | ||
#ifdef BPF_F_REPLACE | ||
if (errno == EINVAL && replacefd >= 0) | ||
{ | ||
skip_replace = true; | ||
continue; | ||
} | ||
#endif | ||
return crun_make_error (err, errno, "bpf attach"); | ||
} | ||
|
||
/* Now that the new program is installed, remove all the programs that were previously installed. */ | ||
if (replacefd < 0 && n_progs) | ||
return remove_all_progs (dirfd, progs, n_progs, err); | ||
|
||
return 0; | ||
} | ||
} | ||
|
||
int | ||
libcrun_ebpf_load (struct bpf_program *program, int dirfd, const char *pin, libcrun_error_t *err) | ||
{ | ||
#ifndef HAVE_EBPF | ||
return crun_make_error (err, 0, "eBPF not supported"); | ||
#else | ||
int fd, ret; | ||
cleanup_close int fd = -1; | ||
union bpf_attr attr; | ||
struct rlimit limit; | ||
int ret; | ||
|
||
limit.rlim_cur = RLIM_INFINITY; | ||
limit.rlim_max = RLIM_INFINITY; | ||
|
@@ -329,30 +483,26 @@ libcrun_ebpf_load (struct bpf_program *program, int dirfd, const char *pin, libc | |
|
||
fd = bpf (BPF_PROG_LOAD, &attr, sizeof (attr)); | ||
if (fd < 0) | ||
return crun_make_error (err, errno, "bpf create %s", log); | ||
return crun_make_error (err, errno, "bpf create `%s`", log); | ||
} | ||
|
||
memset (&attr, 0, sizeof (attr)); | ||
attr.attach_type = BPF_CGROUP_DEVICE; | ||
attr.target_fd = dirfd; | ||
attr.attach_bpf_fd = fd; | ||
attr.attach_flags = BPF_F_ALLOW_MULTI; | ||
|
||
ret = bpf (BPF_PROG_ATTACH, &attr, sizeof (attr)); | ||
if (ret < 0) | ||
return crun_make_error (err, errno, "bpf attach"); | ||
ret = ebpf_attach_program (fd, dirfd, err); | ||
if (UNLIKELY (ret < 0)) | ||
return ret; | ||
|
||
/* Optionally pin the program to the specified path. */ | ||
if (pin) | ||
{ | ||
unlink (pin); | ||
|
||
memset (&attr, 0, sizeof (attr)); | ||
attr.pathname = (uint64_t) pin; | ||
attr.bpf_fd = fd; | ||
ret = bpf (BPF_OBJ_PIN, &attr, sizeof (attr)); | ||
if (ret < 0) | ||
return crun_make_error (err, errno, "bpf pin to %s", pin); | ||
return crun_make_error (err, errno, "bpf pin to `%s`", pin); | ||
} | ||
|
||
return fd; | ||
return 0; | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
attempt = 0; attempt < MAX_ATTEMPTS; attempt++
for readabilityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to rewrite using the condition inside of the for loop but there is some extra logic that decides whether to retry or not.