Remove bpfilter daemon and consolidate every feature in libbpfilter#467
Remove bpfilter daemon and consolidate every feature in libbpfilter#467qdeslandes wants to merge 6 commits intofacebook:mainfrom
Conversation
The cgen/, bpf/, ctx, and xlate modules were compiled directly into the bpfilter binary. Move them into libbpfilter so the filtering logic lives entirely in the shared library and the daemon is a thin entry point.
Replace the daemon IPC path in libbpfilter/cli.c with direct calls to the cgen and ctx APIs, allowing bfcli to load, attach, and manage chains without a running daemon. Move ctx and elfstub headers to the public libbpfilter API, pull BTF lifetime into bf_ctx_setup/teardown, remove namespace switching and transient mode, and extend bfcli's stage-1 parser with global flags (--verbose, --bpffs-path, --with-bpf-token, deprecated compat options).
Replace daemon e2e tests with namespace and persistence tests that
exercise bpffs-based chain discovery directly, without starting or
stopping the daemon process.
Remove the daemon lifecycle (start_bpfilter/stop_bpfilter) from the
test harness and propagate --bpffs-path through the ${BFCLI} variable
so all test invocations use the sandbox-local bpffs mount.
Skip bf_ctx_setup in bfcli when --dry-run is used so matcher tests
no longer require bpffs access. Fix clang-analyzer BitwiseShift
warning in --verbose option parsing.
Drop namespace unit tests and request namespace accessors that are no
longer part of the public API.
The daemon has been removed in favour of direct library calls. Update comments, documentation, and user-facing strings that still referred to "the daemon" so the terminology is consistent with the new architecture. External kernel headers (src/external/) are left unchanged.
Lock bpfilter directory in bpffs to prevent concurrent access to the chains and BPF objects.
`make test` runs all the tests, including fuzzing the bfcli parser for 60 seconds (`make fuzzing` target). This operation takes more time than all the other tests combined which providing little value (fuzzing for a few hours could be more interesting). Disable fuzzing in the `make test` target.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| _bf_global_ctx = _ctx; | ||
|
|
||
| if (!bf_ctx_is_transient()) { | ||
| r = _bf_ctx_discover(); | ||
| if (r) { | ||
| _bf_ctx_free(&_bf_global_ctx); | ||
| return bf_err_r(r, "failed to discover chains"); | ||
| } | ||
| pindir_fd = bf_ctx_get_pindir_fd(); | ||
| if (pindir_fd < 0) | ||
| return bf_err_r(pindir_fd, "failed to get pin directory FD"); |
There was a problem hiding this comment.
Claude: Dangling _bf_global_ctx on new error paths (must-fix)
Commit 5 adds two early-return paths (here and line 401) after _bf_global_ctx = _ctx (line 393) but before TAKE_PTR(_ctx) (line 411). When either return fires, the _free_bf_ctx_ cleanup on _ctx frees the context object, but _bf_global_ctx still points to the freed memory. Any subsequent access (e.g. a retry of bf_ctx_setup(), or bf_ctx_teardown()) is use-after-free.
The _bf_ctx_discover() error path (line 405) already handles this correctly with _bf_ctx_free(&_bf_global_ctx).
Possible fixes:
- Move
_bf_global_ctx = _ctxto just beforeTAKE_PTR(_ctx)(after all fallible operations), or - Add
_bf_global_ctx = NULLbefore each early return on the new paths.
| static int copy_hookopts(struct bf_hookopts **dest, | ||
| const struct bf_hookopts *src) |
There was a problem hiding this comment.
Claude: Naming convention violation (must-fix)
Static functions copy_hookopts (line 27) and copy_set (line 377) lack the required _bf_ prefix for internal functions in libbpfilter. Every other static function in the library follows this convention (e.g. _bf_ctx_free, _bf_ctx_gen_token, _bf_rule_references_empty_set).
| static int copy_hookopts(struct bf_hookopts **dest, | |
| const struct bf_hookopts *src) | |
| static int _bf_copy_hookopts(struct bf_hookopts **dest, |
| } | ||
|
|
||
| int bf_chain_update(const struct bf_chain *chain) | ||
| static int copy_set(struct bf_set **dest, const struct bf_set *src) |
There was a problem hiding this comment.
Claude: Naming convention violation (must-fix)
Same as copy_hookopts above -- should be _bf_copy_set.
| static int copy_set(struct bf_set **dest, const struct bf_set *src) | |
| static int _bf_copy_set(struct bf_set **dest, const struct bf_set *src) |
| Environment variables | ||
| --------------------- | ||
|
|
||
| ``bfcli`` reads the following environment variables to configure the library before processing commands. These variables are ignored when ``--dry-run`` is used. | ||
|
|
There was a problem hiding this comment.
Claude: "Environment variables" section is inaccurate (must-fix)
BF_BPFFS_PATH, BF_WITH_BPF_TOKEN, and BF_VERBOSE are not environment variables -- they are command-line options (--bpffs-path, --with-bpf-token, --verbose) parsed via argp in src/bfcli/opts.c. There are no getenv() calls in src/bfcli/ or src/libbpfilter/.
The e2e test harness does export BF_BPFFS_PATH=... but only uses it as a shell variable to construct the --bpffs-path CLI argument in e2e_test_util.sh.
This section should be renamed (e.g. "Configuration options") and rewritten to document the actual CLI flags (--bpffs-path PATH, --with-bpf-token, --verbose FLAG).
Additionally, BF_VERBOSE is described as a "comma-separated list", but the code in opts.c (lines 362-374) parses each --verbose invocation as a single enum value. Users must pass --verbose multiple times (e.g., --verbose debug --verbose bpf).
| ------------- | ||
|
|
||
| From here on, we assume **bpfilter** has been installed on your system. If you build it locally, you will need to substitute the ``bpfilter`` command with ``$BUILD_DIR/output/sbin/bpfilter``, same for ``bfcli``. The example below is meant to familiarize you with **bpfilter**, more in-depth information can be found throughout the documentation. | ||
| From here on, we assume **bpfilter** has been installed on your system. If you build it locally, you will need to substitute the ``bfcli`` command with ``$BUILD_DIR/output/bin/bfcli``. The example below is meant to familiarize you with **bpfilter**, more in-depth information can be found throughout the documentation. |
There was a problem hiding this comment.
Claude: Wrong binary path (must-fix)
$BUILD_DIR/output/bin/bfcli should be $BUILD_DIR/output/sbin/bfcli. CMAKE_RUNTIME_OUTPUT_DIRECTORY is set to ${CMAKE_BINARY_DIR}/output/sbin (root CMakeLists.txt line 59), and bfcli installs to CMAKE_INSTALL_SBINDIR (src/bfcli/CMakeLists.txt line 81). The README.md already uses the correct sbin path.
| /// BPF token file descriptor | ||
| int token_fd; | ||
|
|
||
| int lock_fd; |
There was a problem hiding this comment.
Claude: Missing field documentation (suggestion)
Adjacent fields (token_fd, with_bpf_token, bpffs_path, verbose) all have /// Doxygen comments. Consider adding one here for consistency, e.g.:
| int lock_fd; | |
| /// File descriptor for the exclusive bpffs lock | |
| int lock_fd; |
|
|
||
| r = flock(pindir_fd, LOCK_EX | LOCK_NB); | ||
| if (r) | ||
| return bf_err_r(-errno, "failed to lock pin directory"); |
There was a problem hiding this comment.
Claude: Unhelpful error message for contention case (suggestion)
When flock() fails with EWOULDBLOCK (another bfcli instance holds the lock), the message "failed to lock pin directory" doesn't indicate contention. Since this is now the primary coordination mechanism (replacing the daemon's single-instance guarantee), a more specific message would aid debugging, e.g.: "pin directory is locked, another bfcli instance may be running".
Claude PR Review — #467PR: Remove bpfilter daemon and consolidate every feature in libbpfilter SummaryThis PR eliminates the bpfilter daemon, moving all filtering logic into libbpfilter as a shared library. bfcli now calls libbpfilter directly. The IPC layer (request/response/io/ns) is removed, e2e tests are adapted, and all "daemon" references are scrubbed from code and docs. Commit 5 adds flock-based locking on the bpffs pin directory for concurrency control. Commit 6 excludes fuzzing from FindingsMust-fix (4)
Suggestions (2)
Commit messages (2)
Minor (3, not posted inline)
Overall Status: FAIL4 must-fix issues block merge. The dangling Reviewed at |
1 participant
As it is now, this PR can be merged into
main, but more polishing will be done before tagging a new release.