Skip to content

Commit fe72605

Browse files
committed
fix(fact-ebpf): improve kernel compatibility
In the context of ROX-30439, I started testing fact on some RHCOS versions and found some issues that are addressed by this PR, namely: * The relocation of kernfs_node for the renamed parent field was wrong, causing cgroups to not be captured correctly. * Some missing null checks that are not necessary on newer kernels. * path_unlink LSM hook is not allowed to call bpf_d_path on older kernels. This last point is the most involved part of the PR, I couldn't find a precise way to check if this call is allowed on the running kernel, so instead I created a small checks BPF object that can try to load a small probe and check if the load was successful, then use a constant loaded at runtime to exclude the call when needed.
1 parent 36772c7 commit fe72605

File tree

14 files changed

+263
-114
lines changed

14 files changed

+263
-114
lines changed

fact-ebpf/build.rs

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,35 +6,41 @@ use std::{
66
};
77

88
fn compile_bpf(out_dir: &Path) -> anyhow::Result<()> {
9-
let obj = match out_dir.join("main.o").into_os_string().into_string() {
10-
Ok(s) => s,
11-
Err(os_string) => anyhow::bail!("Failed to convert path to string {:?}", os_string),
12-
};
13-
149
let target_arch = format!("-D__TARGET_ARCH_{}", env::var("CARGO_CFG_TARGET_ARCH")?);
10+
let base_args = [
11+
"-target",
12+
"bpf",
13+
"-O2",
14+
"-g",
15+
"-c",
16+
"-Wall",
17+
"-Werror",
18+
&target_arch,
19+
];
20+
21+
for name in ["main", "checks"] {
22+
let obj = match out_dir
23+
.join(format!("{name}.o"))
24+
.into_os_string()
25+
.into_string()
26+
{
27+
Ok(s) => s,
28+
Err(os_string) => anyhow::bail!("Failed to convert path to string {:?}", os_string),
29+
};
1530

16-
match Command::new("clang")
17-
.args([
18-
"-target",
19-
"bpf",
20-
"-O2",
21-
"-g",
22-
"-c",
23-
"-Wall",
24-
"-Werror",
25-
&target_arch,
26-
"src/bpf/main.c",
27-
"-o",
28-
&obj,
29-
])
30-
.status()
31-
{
32-
Ok(status) => {
33-
if !status.success() {
34-
anyhow::bail!("Failed to compile eBPF. See stderr for details.");
31+
match Command::new("clang")
32+
.args(base_args)
33+
.arg(format!("src/bpf/{name}.c"))
34+
.args(["-o", &obj])
35+
.status()
36+
{
37+
Ok(status) => {
38+
if !status.success() {
39+
anyhow::bail!("Failed to compile eBPF. See stderr for details.");
40+
}
3541
}
42+
Err(e) => anyhow::bail!("Failed to execute clang: {}", e),
3643
}
37-
Err(e) => anyhow::bail!("Failed to execute clang: {}", e),
3844
}
3945
Ok(())
4046
}

fact-ebpf/src/bpf/bound_path.h

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// clang-format off
44
#include "types.h"
55
#include "maps.h"
6+
#include "d_path.h"
67

78
#include "vmlinux.h"
89

@@ -21,10 +22,13 @@ __always_inline static void path_write_char(char* p, unsigned int offset, char c
2122
*path_safe_access(p, offset) = c;
2223
}
2324

24-
__always_inline static struct bound_path_t* path_read(struct path* path) {
25+
__always_inline static struct bound_path_t* _path_read(struct path* path, bool use_bpf_d_path) {
2526
struct bound_path_t* bound_path = get_bound_path();
27+
if (bound_path == NULL) {
28+
return NULL;
29+
}
2630

27-
bound_path->len = bpf_d_path(path, bound_path->path, PATH_MAX);
31+
bound_path->len = use_bpf_d_path ? bpf_d_path(path, bound_path->path, PATH_MAX) : d_path(path, bound_path->path, PATH_MAX);
2832
if (bound_path->len <= 0) {
2933
return NULL;
3034
}
@@ -35,6 +39,14 @@ __always_inline static struct bound_path_t* path_read(struct path* path) {
3539
return bound_path;
3640
}
3741

42+
__always_inline static struct bound_path_t* path_read(struct path* path) {
43+
return _path_read(path, true);
44+
}
45+
46+
__always_inline static struct bound_path_t* path_read_alternate(struct path* path) {
47+
return _path_read(path, false);
48+
}
49+
3850
enum path_append_status_t {
3951
PATH_APPEND_SUCCESS = 0,
4052
PATH_APPEND_INVALID_LENGTH,

fact-ebpf/src/bpf/checks.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// clang-format off
2+
#include "vmlinux.h"
3+
4+
#include "bound_path.h"
5+
6+
#include <bpf/bpf_helpers.h>
7+
#include <bpf/bpf_tracing.h>
8+
// clang-format on
9+
10+
SEC("lsm/path_unlink")
11+
int BPF_PROG(check_path_unlink_supports_bpf_d_path, struct path* dir, struct dentry* dentry) {
12+
struct bound_path_t* p = path_read(dir);
13+
bpf_printk("dir: %s", p->path);
14+
return 0;
15+
}

fact-ebpf/src/bpf/d_path.h

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
#pragma once
2+
3+
// clang-format off
4+
#include "maps.h"
5+
#include "vmlinux.h"
6+
7+
#include <bpf/bpf_helpers.h>
8+
#include <bpf/bpf_core_read.h>
9+
// clang-format on
10+
11+
/**
12+
* Reimplementation of the kernel d_path function.
13+
*
14+
* We should attempt to use bpf_d_path when possible, but you can't on
15+
* values that have been read using the bpf_probe_* helpers.
16+
*/
17+
__always_inline static long d_path(const struct path* path, char* buf, int buflen) {
18+
if (buflen <= 0) {
19+
return -1;
20+
}
21+
22+
struct helper_t* helper = get_helper();
23+
if (helper == NULL) {
24+
return -1;
25+
}
26+
27+
struct task_struct* task = (struct task_struct*)bpf_get_current_task();
28+
int offset = (buflen - 1) & (PATH_MAX - 1);
29+
helper->buf[offset] = '\0'; // Ensure null termination
30+
31+
struct path root;
32+
BPF_CORE_READ_INTO(&root, task, fs, root);
33+
struct mount* mnt = container_of(path->mnt, struct mount, mnt);
34+
struct dentry* dentry = BPF_CORE_READ(path, dentry);
35+
36+
for (int i = 0; i < 16 && (dentry != root.dentry || &mnt->mnt != root.mnt); i++) {
37+
struct dentry* parent = BPF_CORE_READ(dentry, d_parent);
38+
struct dentry* mnt_root = BPF_CORE_READ(mnt, mnt.mnt_root);
39+
40+
if (dentry == mnt_root) {
41+
struct mount* m = BPF_CORE_READ(mnt, mnt_parent);
42+
if (m != mnt) {
43+
dentry = BPF_CORE_READ(mnt, mnt_mountpoint);
44+
mnt = m;
45+
continue;
46+
}
47+
break;
48+
}
49+
50+
if (dentry == parent) {
51+
return -1;
52+
}
53+
54+
struct qstr d_name;
55+
BPF_CORE_READ_INTO(&d_name, dentry, d_name);
56+
int len = d_name.len;
57+
if (len <= 0 || len >= buflen) {
58+
return -1;
59+
}
60+
61+
offset -= len;
62+
if (offset <= 0) {
63+
return -1;
64+
}
65+
66+
if (bpf_probe_read_kernel(&helper->buf[offset], len, d_name.name) != 0) {
67+
return -1;
68+
}
69+
70+
offset--;
71+
if (offset <= 0) {
72+
return -1;
73+
}
74+
helper->buf[offset] = '/';
75+
76+
dentry = parent;
77+
}
78+
79+
bpf_probe_read_str(buf, buflen, &helper->buf[offset]);
80+
return buflen - offset;
81+
}

fact-ebpf/src/bpf/events.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#pragma once
2+
13
#include <bpf/bpf_helpers.h>
24

35
#include "maps.h"
@@ -17,6 +19,10 @@ __always_inline static void submit_event(struct metrics_by_hook_t* m, file_activ
1719
bpf_probe_read_str(event->filename, PATH_MAX, filename);
1820

1921
struct helper_t* helper = get_helper();
22+
if (helper == NULL) {
23+
goto error;
24+
}
25+
2026
const char* p = get_host_path(helper->buf, dentry);
2127
if (p != NULL) {
2228
bpf_probe_read_str(event->host_file, PATH_MAX, p);

fact-ebpf/src/bpf/file.h

Lines changed: 1 addition & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -5,79 +5,14 @@
55

66
#include "bound_path.h"
77
#include "builtins.h"
8+
#include "d_path.h"
89
#include "types.h"
910
#include "maps.h"
1011

1112
#include <bpf/bpf_helpers.h>
1213
#include <bpf/bpf_core_read.h>
1314
// clang-format on
1415

15-
/**
16-
* Reimplementation of the kernel d_path function.
17-
*
18-
* We should attempt to use bpf_d_path when possible, but you can't on
19-
* values that have been read using the bpf_probe_* helpers.
20-
*/
21-
__always_inline static char* d_path(const struct path* path, char* buf, int buflen) {
22-
if (buflen <= 0) {
23-
return NULL;
24-
}
25-
26-
struct task_struct* task = (struct task_struct*)bpf_get_current_task();
27-
int offset = (buflen - 1) & (PATH_MAX - 1);
28-
buf[offset] = '\0'; // Ensure null termination
29-
30-
struct path root;
31-
BPF_CORE_READ_INTO(&root, task, fs, root);
32-
struct mount* mnt = container_of(path->mnt, struct mount, mnt);
33-
struct dentry* dentry = BPF_CORE_READ(path, dentry);
34-
35-
for (int i = 0; i < 16 && (dentry != root.dentry || &mnt->mnt != root.mnt); i++) {
36-
struct dentry* parent = BPF_CORE_READ(dentry, d_parent);
37-
struct dentry* mnt_root = BPF_CORE_READ(mnt, mnt.mnt_root);
38-
39-
if (dentry == mnt_root) {
40-
struct mount* m = BPF_CORE_READ(mnt, mnt_parent);
41-
if (m != mnt) {
42-
dentry = BPF_CORE_READ(mnt, mnt_mountpoint);
43-
mnt = m;
44-
continue;
45-
}
46-
return &buf[offset];
47-
}
48-
49-
if (dentry == parent) {
50-
return NULL;
51-
}
52-
53-
struct qstr d_name;
54-
BPF_CORE_READ_INTO(&d_name, dentry, d_name);
55-
int len = d_name.len;
56-
if (len <= 0 || len >= buflen) {
57-
return NULL;
58-
}
59-
60-
offset -= len;
61-
if (offset <= 0) {
62-
return NULL;
63-
}
64-
65-
if (bpf_probe_read_kernel(&buf[offset], len, d_name.name) != 0) {
66-
return NULL;
67-
}
68-
69-
offset--;
70-
if (offset <= 0) {
71-
return NULL;
72-
}
73-
buf[offset] = '/';
74-
75-
dentry = parent;
76-
}
77-
78-
return &buf[offset];
79-
}
80-
8116
__always_inline static char* get_host_path(char buf[PATH_MAX * 2], struct dentry* d) {
8217
int offset = PATH_MAX - 1;
8318
buf[PATH_MAX - 1] = '\0';

fact-ebpf/src/bpf/main.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
// clang-format off
2+
#include "vmlinux.h"
3+
24
#include "file.h"
35
#include "types.h"
46
#include "process.h"
57
#include "maps.h"
68
#include "events.h"
79
#include "bound_path.h"
810

9-
#include "vmlinux.h"
10-
1111
#include <bpf/bpf_helpers.h>
1212
#include <bpf/bpf_tracing.h>
1313
#include <bpf/bpf_core_read.h>
@@ -22,6 +22,9 @@ char _license[] SEC("license") = "Dual MIT/GPL";
2222
SEC("lsm/file_open")
2323
int BPF_PROG(trace_file_open, struct file* file) {
2424
struct metrics_t* m = get_metrics();
25+
if (m == NULL) {
26+
return 0;
27+
}
2528

2629
m->file_open.total++;
2730

@@ -58,10 +61,19 @@ int BPF_PROG(trace_file_open, struct file* file) {
5861
SEC("lsm/path_unlink")
5962
int BPF_PROG(trace_path_unlink, struct path* dir, struct dentry* dentry) {
6063
struct metrics_t* m = get_metrics();
64+
if (m == NULL) {
65+
return 0;
66+
}
6167

6268
m->path_unlink.total++;
6369

64-
struct bound_path_t* path = path_read(dir);
70+
struct bound_path_t* path = NULL;
71+
if (path_unlink_supports_bpf_d_path) {
72+
path = path_read(dir);
73+
} else {
74+
path = path_read_alternate(dir);
75+
}
76+
6577
if (path == NULL) {
6678
bpf_printk("Failed to read path");
6779
goto error;

fact-ebpf/src/bpf/maps.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@ struct {
3838
__always_inline static bool filter_by_prefix() {
3939
unsigned int zero = 0;
4040
char* res = bpf_map_lookup_elem(&filter_by_prefix_map, &zero);
41-
return *res != 0;
41+
42+
// The NULL check is simply here to satisfy some verifiers, the result
43+
// will never actually be NULL.
44+
return res == NULL || *res != 0;
4245
}
4346

4447
struct {
@@ -82,6 +85,7 @@ __always_inline static struct bound_path_t* get_bound_path() {
8285

8386
struct {
8487
__uint(type, BPF_MAP_TYPE_RINGBUF);
88+
__uint(max_entries, 8 * 1024 * 1024);
8589
} rb SEC(".maps");
8690

8791
struct {
@@ -97,5 +101,6 @@ __always_inline static struct metrics_t* get_metrics() {
97101
}
98102

99103
uint64_t host_mount_ns;
104+
volatile const bool path_unlink_supports_bpf_d_path;
100105

101106
// clang-format on

0 commit comments

Comments
 (0)