Skip to content

fix(fact-ebpf): improve kernel compatibility #125

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 17, 2025
Merged

fix(fact-ebpf): improve kernel compatibility #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
366015d
fix(fact-ebpf): improve kernel compatibility
Molter73 Oct 22, 2025
85e17ef
Rename path_read_alternate to path_read_no_d_path
Molter73 Oct 31, 2025
b801e91
Add small comment on filter_by_prefix_map usage
Molter73 Nov 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 31 additions & 25 deletions fact-ebpf/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,41 @@ use std::{
};

fn compile_bpf(out_dir: &Path) -> anyhow::Result<()> {
let obj = match out_dir.join("main.o").into_os_string().into_string() {
Ok(s) => s,
Err(os_string) => anyhow::bail!("Failed to convert path to string {:?}", os_string),
};

let target_arch = format!("-D__TARGET_ARCH_{}", env::var("CARGO_CFG_TARGET_ARCH")?);
let base_args = [
"-target",
"bpf",
"-O2",
"-g",
"-c",
"-Wall",
"-Werror",
&target_arch,
];

for name in ["main", "checks"] {
let obj = match out_dir
.join(format!("{name}.o"))
.into_os_string()
.into_string()
{
Ok(s) => s,
Err(os_string) => anyhow::bail!("Failed to convert path to string {:?}", os_string),
};

match Command::new("clang")
.args([
"-target",
"bpf",
"-O2",
"-g",
"-c",
"-Wall",
"-Werror",
&target_arch,
"src/bpf/main.c",
"-o",
&obj,
])
.status()
{
Ok(status) => {
if !status.success() {
anyhow::bail!("Failed to compile eBPF. See stderr for details.");
match Command::new("clang")
.args(base_args)
.arg(format!("src/bpf/{name}.c"))
.args(["-o", &obj])
.status()
{
Ok(status) => {
if !status.success() {
anyhow::bail!("Failed to compile eBPF. See stderr for details.");
}
}
Err(e) => anyhow::bail!("Failed to execute clang: {}", e),
}
Err(e) => anyhow::bail!("Failed to execute clang: {}", e),
}
Ok(())
}
Expand Down
16 changes: 14 additions & 2 deletions fact-ebpf/src/bpf/bound_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// clang-format off
#include "types.h"
#include "maps.h"
#include "d_path.h"

#include "vmlinux.h"

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

__always_inline static struct bound_path_t* path_read(struct path* path) {
__always_inline static struct bound_path_t* _path_read(struct path* path, bool use_bpf_d_path) {
Copy link
Contributor

@erthalion erthalion Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to have a second though on how much this is needed. We did not plan originally to have path_unlink in the first milestone, even more so backward compatible implementation for older kernels. At the same time the implementation complexity seems to be quite significant because of that. How about keep it tight, and record lack of path_unlink support for older versions as a known limitation?

Copy link
Collaborator Author

@Molter73 Molter73 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not plan originally to have path_unlink in the first milestone

I know, but at this point, the distinction between m1 and m2 is pretty blurry + we are running out of time and we need to get things done, that includes implementing at least a basic set of file operations IMO.

even more so backward compatible implementation for older kernels

The changes are needed for fact to be compatible with RHCOS 4.15 and 4.18 (RHEL 9), so it's not like I'm backporting to ancient kernels, this is very much versions being used in production right now.

At the same time the implementation complexity seems to be quite significant because of that.

I wouldn't consider loading a small probe to check whether we can use bpf_d_path in path_unlink hooks and setting a global variable to be too complex. If you are concerned about the d_path implementation, that was already implemented, I just moved it to its own file because I was getting circular includes and moving it out to its own header was the most straightforward solution. This implementation is also needed to get the exe_path from the process, so it's not like we can just get rid of it.

How about keep it tight, and record lack of path_unlink support for older versions as a known limitation?

Problem with this is we don't just not have path_unlink support, the probe is rejected by the verifier, so we would still need to at least have the checks in place to know when we should attempt to load path_unlink and when to skip it.

struct bound_path_t* bound_path = get_bound_path();
if (bound_path == NULL) {
return NULL;
}

bound_path->len = bpf_d_path(path, bound_path->path, PATH_MAX);
bound_path->len = use_bpf_d_path ? bpf_d_path(path, bound_path->path, PATH_MAX) : d_path(path, bound_path->path, PATH_MAX);
Copy link
Contributor

@erthalion erthalion Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that bpf_d_path is already legacy :) But for a good reason, looks like there is a possibility of concurrent modification of the path structure, leading to use-after-free -- I assume the same is correct for using copied d_path function as well.

Copy link
Collaborator Author

@Molter73 Molter73 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the copied d_path function is probably worse actually, but it is not like we have much choice ATM. As mentioned in another comment, the newer bpf_path_d_path is only available on pretty new kernels and aya doesn't support KFuncs yet.

if (bound_path->len <= 0) {
return NULL;
}
Expand All @@ -35,6 +39,14 @@ __always_inline static struct bound_path_t* path_read(struct path* path) {
return bound_path;
}

__always_inline static struct bound_path_t* path_read(struct path* path) {
return _path_read(path, true);
}

__always_inline static struct bound_path_t* path_read_no_d_path(struct path* path) {
return _path_read(path, false);
}

enum path_append_status_t {
PATH_APPEND_SUCCESS = 0,
PATH_APPEND_INVALID_LENGTH,
Expand Down
15 changes: 15 additions & 0 deletions fact-ebpf/src/bpf/checks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// clang-format off
#include "vmlinux.h"

#include "bound_path.h"

#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
// clang-format on

SEC("lsm/path_unlink")
int BPF_PROG(check_path_unlink_supports_bpf_d_path, struct path* dir, struct dentry* dentry) {
Copy link
Contributor

@erthalion erthalion Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is another option. From what I understand we know the exact commit (which on bpf-next resolves to v6.7-rc3), where an LSM hook on path_unlink became capable of calling bpf_d_path. How about just doing a condition on the kernel version?

Copy link
Collaborator Author

@Molter73 Molter73 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this, but RHEL 9 uses 5.14.0 for all its kernels, we would need to figure out when/if the change was backported and have a separate check for that, I figured probing was more reliable than that.
(Also, I suck at navigating the kernel mailing list and I couldn't find the exact commit that made this change, thanks for doing that!).

struct bound_path_t* p = path_read(dir);
bpf_printk("dir: %s", p->path);
return 0;
}
81 changes: 81 additions & 0 deletions fact-ebpf/src/bpf/d_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
#pragma once

// clang-format off
#include "maps.h"
#include "vmlinux.h"

#include <bpf/bpf_helpers.h>
#include <bpf/bpf_core_read.h>
// clang-format on

/**
* Reimplementation of the kernel d_path function.
*
* We should attempt to use bpf_d_path when possible, but you can't on
* values that have been read using the bpf_probe_* helpers.
Copy link
Contributor

@erthalion erthalion Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any clarification on why? I assume because bpf_d_path requires a path structure with BTF information, and bpf_probe_* helpers do not provide it, is that so?

Copy link
Collaborator Author

@Molter73 Molter73 Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I try to use bpf_d_path to read the path to the executable path of task like this:

    BPF_CORE_READ_INTO(&path, task, mm, exe_file, f_path);
    bpf_d_path(&path, p->lineage[i].exe_path, PATH_MAX);

I get the following verifier error:

657: (85) call bpf_probe_read_kernel#113      ; R0=scalar() fp-16=mmmmmmmm fp-24=mmmmmmmm refs=4
; bpf_d_path(&path, p->lineage[i].exe_path, PATH_MAX); @ process.h:95
658: (bf) r2 = r9                     ; R2_w=ringbuf_mem(ref_obj_id=4,sz=28736) R9=ringbuf_mem(ref_obj_id=4,sz=28736) refs=4
659: (07) r2 += 12336                 ; R2_w=ringbuf_mem(ref_obj_id=4,off=12336,sz=28736) refs=4
660: (bf) r1 = r6                     ; R1_w=fp-24 R6=fp-24 refs=4
661: (b4) w3 = 4096                   ; R3_w=4096 refs=4
662: (85) call bpf_d_path#147
R1 type=fp expected=ptr_, trusted_ptr_, rcu_ptr_
verification time 4729 usec
stack depth 128
processed 1731 insns (limit 1000000) max_states_per_insn 4 total_states 68 peak_states 68 mark_read 64

Searching for the error I found this answer on SO: https://stackoverflow.com/a/79348591

So it seems that you can't use variables in the stack with bpf_d_path, but these are very much needed when accessing things with bpf_probe_read_*, so we need to use our own implementation of d_path, which sucks.

On newer kernels we can probably use bpf_path_d_path which may help... Except aya doesn't support KFuncs yet, so 🤷🏻

*/
__always_inline static long d_path(const struct path* path, char* buf, int buflen) {
if (buflen <= 0) {
return -1;
}

struct helper_t* helper = get_helper();
if (helper == NULL) {
return -1;
}

struct task_struct* task = (struct task_struct*)bpf_get_current_task();
int offset = (buflen - 1) & (PATH_MAX - 1);
helper->buf[offset] = '\0'; // Ensure null termination

struct path root;
BPF_CORE_READ_INTO(&root, task, fs, root);
struct mount* mnt = container_of(path->mnt, struct mount, mnt);
struct dentry* dentry = BPF_CORE_READ(path, dentry);

for (int i = 0; i < 16 && (dentry != root.dentry || &mnt->mnt != root.mnt); i++) {
struct dentry* parent = BPF_CORE_READ(dentry, d_parent);
struct dentry* mnt_root = BPF_CORE_READ(mnt, mnt.mnt_root);

if (dentry == mnt_root) {
struct mount* m = BPF_CORE_READ(mnt, mnt_parent);
if (m != mnt) {
dentry = BPF_CORE_READ(mnt, mnt_mountpoint);
mnt = m;
continue;
}
break;
}

if (dentry == parent) {
return -1;
}

struct qstr d_name;
BPF_CORE_READ_INTO(&d_name, dentry, d_name);
int len = d_name.len;
if (len <= 0 || len >= buflen) {
return -1;
}

offset -= len;
if (offset <= 0) {
return -1;
}

if (bpf_probe_read_kernel(&helper->buf[offset], len, d_name.name) != 0) {
return -1;
}

offset--;
if (offset <= 0) {
return -1;
}
helper->buf[offset] = '/';

dentry = parent;
}

bpf_probe_read_str(buf, buflen, &helper->buf[offset]);
return buflen - offset;
}
6 changes: 6 additions & 0 deletions fact-ebpf/src/bpf/events.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#pragma once

#include <bpf/bpf_helpers.h>

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

struct helper_t* helper = get_helper();
if (helper == NULL) {
goto error;
}

const char* p = get_host_path(helper->buf, dentry);
if (p != NULL) {
bpf_probe_read_str(event->host_file, PATH_MAX, p);
Expand Down
67 changes: 1 addition & 66 deletions fact-ebpf/src/bpf/file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,79 +5,14 @@

#include "bound_path.h"
#include "builtins.h"
#include "d_path.h"
#include "types.h"
#include "maps.h"

#include <bpf/bpf_helpers.h>
#include <bpf/bpf_core_read.h>
// clang-format on

/**
* Reimplementation of the kernel d_path function.
*
* We should attempt to use bpf_d_path when possible, but you can't on
* values that have been read using the bpf_probe_* helpers.
*/
__always_inline static char* d_path(const struct path* path, char* buf, int buflen) {
if (buflen <= 0) {
return NULL;
}

struct task_struct* task = (struct task_struct*)bpf_get_current_task();
int offset = (buflen - 1) & (PATH_MAX - 1);
buf[offset] = '\0'; // Ensure null termination

struct path root;
BPF_CORE_READ_INTO(&root, task, fs, root);
struct mount* mnt = container_of(path->mnt, struct mount, mnt);
struct dentry* dentry = BPF_CORE_READ(path, dentry);

for (int i = 0; i < 16 && (dentry != root.dentry || &mnt->mnt != root.mnt); i++) {
struct dentry* parent = BPF_CORE_READ(dentry, d_parent);
struct dentry* mnt_root = BPF_CORE_READ(mnt, mnt.mnt_root);

if (dentry == mnt_root) {
struct mount* m = BPF_CORE_READ(mnt, mnt_parent);
if (m != mnt) {
dentry = BPF_CORE_READ(mnt, mnt_mountpoint);
mnt = m;
continue;
}
return &buf[offset];
}

if (dentry == parent) {
return NULL;
}

struct qstr d_name;
BPF_CORE_READ_INTO(&d_name, dentry, d_name);
int len = d_name.len;
if (len <= 0 || len >= buflen) {
return NULL;
}

offset -= len;
if (offset <= 0) {
return NULL;
}

if (bpf_probe_read_kernel(&buf[offset], len, d_name.name) != 0) {
return NULL;
}

offset--;
if (offset <= 0) {
return NULL;
}
buf[offset] = '/';

dentry = parent;
}

return &buf[offset];
}

__always_inline static char* get_host_path(char buf[PATH_MAX * 2], struct dentry* d) {
int offset = PATH_MAX - 1;
buf[PATH_MAX - 1] = '\0';
Expand Down
18 changes: 15 additions & 3 deletions fact-ebpf/src/bpf/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// clang-format off
#include "vmlinux.h"

#include "file.h"
#include "types.h"
#include "process.h"
#include "maps.h"
#include "events.h"
#include "bound_path.h"

#include "vmlinux.h"

#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
#include <bpf/bpf_core_read.h>
Expand All @@ -22,6 +22,9 @@ char _license[] SEC("license") = "Dual MIT/GPL";
SEC("lsm/file_open")
int BPF_PROG(trace_file_open, struct file* file) {
struct metrics_t* m = get_metrics();
if (m == NULL) {
return 0;
}

m->file_open.total++;

Expand Down Expand Up @@ -58,10 +61,19 @@ int BPF_PROG(trace_file_open, struct file* file) {
SEC("lsm/path_unlink")
int BPF_PROG(trace_path_unlink, struct path* dir, struct dentry* dentry) {
struct metrics_t* m = get_metrics();
if (m == NULL) {
return 0;
}

m->path_unlink.total++;

struct bound_path_t* path = path_read(dir);
struct bound_path_t* path = NULL;
if (path_unlink_supports_bpf_d_path) {
path = path_read(dir);
} else {
path = path_read_no_d_path(dir);
}

if (path == NULL) {
bpf_printk("Failed to read path");
goto error;
Expand Down
11 changes: 10 additions & 1 deletion fact-ebpf/src/bpf/maps.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ __always_inline static struct helper_t* get_helper() {
return bpf_map_lookup_elem(&helper_map, &zero);
}

/**
* A map with a single entry, determining whether prefix filtering
* should be done based on the `path_prefix` map.
*/
struct {
__uint(type, BPF_MAP_TYPE_ARRAY);
__type(key, __u32);
Expand All @@ -38,7 +42,10 @@ struct {
__always_inline static bool filter_by_prefix() {
unsigned int zero = 0;
char* res = bpf_map_lookup_elem(&filter_by_prefix_map, &zero);
return *res != 0;

// The NULL check is simply here to satisfy some verifiers, the result
// will never actually be NULL.
Copy link
Contributor

@erthalion erthalion Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it never be NULL? From what I see the corresponding lookup function can return NULL in some cases.

Copy link
Collaborator Author

@Molter73 Molter73 Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of, this is not the trie map, this is a regular map with a single entry that determines whether the trie map has anything and if it should be used for filtering.

Because the map only has one value and it is allocated when the probe is first loaded, and because we are always asking for that one entry (by using int zero = 0; as the key), the lookup will always return that one element.

This is further validated by the fact this code ran without the null check on newer kernels that are smarter about this kind of thing.

Copy link
Contributor

@erthalion erthalion Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, and suggest now to add few more lines of commentaries about this around the filter_by_prefix_map.

return res == NULL || *res != 0;
}

struct {
Expand Down Expand Up @@ -82,6 +89,7 @@ __always_inline static struct bound_path_t* get_bound_path() {

struct {
__uint(type, BPF_MAP_TYPE_RINGBUF);
__uint(max_entries, 8 * 1024 * 1024);
} rb SEC(".maps");

struct {
Expand All @@ -97,5 +105,6 @@ __always_inline static struct metrics_t* get_metrics() {
}

uint64_t host_mount_ns;
volatile const bool path_unlink_supports_bpf_d_path;

// clang-format on
Loading
Loading