Remove bpfilter daemon and consolidate every feature in libbpfilter#467
Conversation
![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 review of PR #467 (a166431)Must fix
Suggestions
Nits
Commit messages
|
|
|
||
| _ctx = calloc(1, sizeof(*_ctx)); | ||
| if (!_ctx) | ||
| return -ENOMEM; |
There was a problem hiding this comment.
Claude: must-fix: BTF resource leak on calloc failure
bf_btf_setup() is called at line 106 before calloc at line 110. If calloc returns NULL, the function returns -ENOMEM and the _free_bf_ctx_ cleanup fires on _ctx, which is still NULL. _bf_ctx_free sees *ctx == NULL and returns immediately — bf_btf_teardown() is never called, leaking the static BTF data allocated by bf_btf_setup().
Fix: move bf_btf_setup() after the calloc succeeds (so _bf_ctx_free will always reach bf_btf_teardown()), or add an explicit bf_btf_teardown() on the early-return path.
| "BF_REQ_CHAIN_PROG_FD failed"); | ||
|
|
||
| return TAKE_FD(prog_fd); | ||
| return dup(cgen->handle->prog_fd); |
There was a problem hiding this comment.
Claude: must-fix: dup() failure returns -1 instead of negative errno
bf_chain_prog_fd returns dup(cgen->handle->prog_fd) directly. If dup() fails it returns -1 and sets errno, but callers expect a negative errno value on failure (the public API contract in bpfilter.h). -1 is ambiguous because it equals -EPERM.
Suggested fix:
int fd = dup(cgen->handle->prog_fd);
if (fd < 0)
return bf_err_r(-errno, "failed to dup program fd for '%s'", name);
return fd;
| "BF_REQ_CHAIN_LOGS failed"); | ||
|
|
||
| return TAKE_FD(logs_fd); | ||
| return dup(cgen->handle->lmap->fd); |
There was a problem hiding this comment.
Claude: must-fix: dup() failure returns -1 instead of negative errno
Same issue as bf_chain_prog_fd above: bf_chain_logs_fd returns dup(cgen->handle->lmap->fd) directly. If dup() fails, callers receive -1 rather than the expected negative errno.
|
|
||
| _ctx->transient = transient; | ||
| _ctx->with_bpf_token = with_bpf_token; | ||
| _ctx->bpffs_path = bpffs_path; |
There was a problem hiding this comment.
Claude: suggestion: bpffs_path stored as a borrowed pointer without copy or documented lifetime
_bf_ctx_new stores the caller's bpffs_path pointer directly (_ctx->bpffs_path = bpffs_path) without copying it. The pointer is used throughout the context's lifetime for directory operations (bf_ctx_get_pindir_fd, bf_ctx_rm_pindir, _bf_ctx_discover).
In the current bfcli usage this is safe (pointer comes from argv), but bf_ctx_setup is now a public API. A third-party caller could pass a stack buffer or freed string. Either strdup the path in _bf_ctx_new (freeing in _bf_ctx_free), or document the lifetime requirement in ctx.h.
|
Review from Codex:
|
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.
qdeslandes
force-pushed
the
daemon-less
branch
2 times, most recently
from
9da2754 to
3201bfa
Compare
| assert(ctx); | ||
| assert(bpffs_path); | ||
|
|
||
| r = bf_btf_setup(); |
There was a problem hiding this comment.
Claude: must-fix: BTF resource leak when calloc fails.
bf_btf_setup() succeeds at line 106, then calloc fails at line 110 and returns -ENOMEM. The _free_bf_ctx_ cleanup on _ctx fires, but _ctx is still NULL so _bf_ctx_free early-returns without calling bf_btf_teardown(). This leaks the struct btf * allocated inside bf_btf_setup.
In the old code, bf_btf_setup/bf_btf_teardown were managed by the daemon's main() — moving them into _bf_ctx_new/_bf_ctx_free introduces this gap. The simplest fix is to move bf_btf_setup() after calloc succeeds, so that any subsequent error path has a non-NULL _ctx and the cleanup attribute will invoke bf_btf_teardown() via _bf_ctx_free.
| #include <bpfilter/ctx.h> | ||
| #include <bpfilter/list.h> | ||
|
|
||
| struct bf_response; |
There was a problem hiding this comment.
Claude: must-fix: Stale struct bf_response; forward declaration.
The bf_response type was removed along with the IPC layer (response.h, response.c). No remaining function in bpfilter.h or anywhere else references this type. This leftover should be removed.
| │ ├── cgen/ # BPF code generation engine | ||
| │ │ ├── matcher/ # Packet matcher codegen (ip4, ip6, tcp, udp, icmp, meta, set) | ||
| │ │ └── prog/ # Program linking (link, map) | ||
| │ ├── xlate.c # Rule translation |
There was a problem hiding this comment.
Claude: must-fix: Directory tree has two issues:
xlate.cis listed (line 22) but this file was removed in this PR. The entry should be deleted.- Lines 23-24 have two
└──connectors at the same indentation level underlibbpfilter/. Only the very last child should use└──; thebpf/entry on line 23 should use├──since*.cfollows it.
| "BF_REQ_CHAIN_PROG_FD failed"); | ||
|
|
||
| return TAKE_FD(prog_fd); | ||
| return dup(cgen->handle->prog_fd); |
There was a problem hiding this comment.
Claude: suggestion: Unchecked dup() return does not follow the project's negative-errno convention.
dup() returns -1 on failure with the actual error in errno. The caller of bf_chain_prog_fd gets -1 with no way to determine the root cause. Every other error path in this function (and across the codebase) returns a specific negative errno. Consider:
int fd = dup(cgen->handle->prog_fd);
if (fd < 0)
return bf_err_r(-errno, "failed to dup program fd for '%s'", name);
return fd;Same applies to bf_chain_logs_fd at line 284 with cgen->handle->lmap->fd.
| .flags = 0, | ||
| .doc = "Path to the bpfilter binary. Defaults to 'bpfilter' in $PATH.", | ||
| .doc = | ||
| "DEPRECATED. Path to the bpfilter binary. Defaults to 'bpfilter' in $PATH.", |
There was a problem hiding this comment.
Claude: suggestion: The --daemon / -d argp option is kept and marked DEPRECATED, but the case 'd': handler was removed from optsParser. If a user passes --daemon <path>, argp dispatches key 'd' to the switch, falls into the default: case, and returns ARGP_ERR_UNKNOWN — printing a confusing "unrecognised option" error rather than a deprecation warning.
Either remove the option entry entirely (since the daemon no longer exists), or add a case 'd': break; so it is silently accepted and ignored.
| * @param transient If true, don't persist state and unload programs on exit. | ||
| * @param with_bpf_token If true, create a BPF token from bpffs. | ||
| * @param bpffs_path Path to the bpffs mountpoint. Can't be NULL. | ||
| * @param verbose Bitmask of verbose flags. |
There was a problem hiding this comment.
Claude: suggestion: The bpffs_path parameter documentation should note the lifetime requirement.
_bf_ctx_new stores bpffs_path as a raw const char * without copying it. Since ctx.h is now a public library header, external callers need to know the string must remain valid for the context's lifetime. Consider adding: The caller must ensure bpffs_path remains valid until bf_ctx_teardown is called.
| :sections: briefdescription detaileddescription typedef struct innerclass enum var define func | ||
|
|
||
|
|
||
| Pack |
There was a problem hiding this comment.
Claude: suggestion: ctx.h is now a public header exposing the library lifecycle (bf_ctx_setup, bf_ctx_teardown, bf_verbose). The removed "Namespaces" section documented ns.h; consider replacing it with a "Context" section containing .. doxygenfile:: bpfilter/ctx.h so these entry-point functions appear in the generated module documentation. Without it, Sphinx's doxygenfile for bpfilter.h won't pull in the transitive ctx.h declarations.
| int run() | ||
| { | ||
| bft::Daemon daemon("bpfilter", bft::Daemon::Options().transient()); | ||
| int r = bf_ctx_setup(false, "/sys/fs/bpf", 0); |
There was a problem hiding this comment.
Claude: suggestion: The bpffs path is hardcoded to "/sys/fs/bpf". The e2e shell tests use a sandbox-specific path (${WORKDIR}/bpf). If the matcher C++ tests ever run in a similar sandboxed environment, this would diverge. Consider extracting the path to a constant or making it configurable for consistency with the shell test infrastructure.
| ${FROM_NS} ${BFCLI} ruleset get | grep "^chain" | awk 'END{exit NR!=1}' | ||
| ${FROM_NS} test -e ${PINNED_PROG} | ||
|
|
||
| # Chain is still discoverable from bpffs after update |
There was a problem hiding this comment.
Claude: nit: Lines 21-22 duplicate lines 18-19 exactly. The comment on line 20 says "Chain is still discoverable from bpffs after update" but the preceding block already verified this immediately after the update. In the old daemon test, this third check was meaningful because it occurred after a daemon restart. Without a restart boundary, the assertion is redundant. Consider either removing it or replacing it with a distinct check (e.g. verifying the updated rule content).
| /// BPF token file descriptor | ||
| int token_fd; | ||
|
|
||
| int lock_fd; |
There was a problem hiding this comment.
Claude: nit: The new lock_fd field has no Doxygen comment. The adjacent token_fd has /// BPF token file descriptor. A brief comment such as /// File descriptor holding an exclusive flock on the pin directory. would clarify the field's purpose.
| #include <string.h> | ||
| #include <unistd.h> | ||
|
|
||
| #include <bpfilter/ctx.h> |
There was a problem hiding this comment.
Claude: nit: Include style inconsistency: #include <bpfilter/ctx.h> uses angle brackets while every other bpfilter/ header in this file (lines 12-19) uses quoted includes ("bpfilter/chain.h", etc.). Within src/libbpfilter/ source files, the existing convention is to use quoted includes for the library's own public headers.
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). Update tests to rely on bfcli only.
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.
| │ │ ├── matcher/ # Packet matcher codegen (ip4, ip6, tcp, udp, icmp, meta, set) | ||
| │ │ └── prog/ # Program linking (link, map) | ||
| │ ├── xlate.c # Rule translation | ||
| │ └── bpf/ # eBPF stub programs |
There was a problem hiding this comment.
Claude: must-fix: Two └── connectors at the same nesting level under libbpfilter/. In tree notation └── means "last child" — only the final sibling should use it. This line (bpf/) should use ├── instead:
| │ └── bpf/ # eBPF stub programs | |
| │ ├── bpf/ # eBPF stub programs |
| "BF_REQ_CHAIN_PROG_FD failed"); | ||
|
|
||
| return TAKE_FD(prog_fd); | ||
| return dup(cgen->handle->prog_fd); |
There was a problem hiding this comment.
Claude: suggestion: dup() returns -1 on failure, but the function contract promises a negative errno value. A bare -1 is ambiguous and callers checking for specific error codes will misbehave. Same issue at line 284 in bf_chain_logs_fd. Consider:
int fd = dup(cgen->handle->prog_fd);
if (fd < 0)
return bf_err_r(-errno, "failed to dup prog fd for '%s'", name);
return fd;
| */ | ||
| int bf_ctx_setup(bool transient, bool with_bpf_token, const char *bpffs_path, | ||
| uint16_t verbose); | ||
| int bf_ctx_setup(bool with_bpf_token, const char *bpffs_path, uint16_t verbose); |
There was a problem hiding this comment.
Claude: suggestion: The bpffs_path pointer is stored without copying (ctx.c does _ctx->bpffs_path = bpffs_path). The Doxygen should note that the pointer must remain valid for the context's lifetime (until bf_ctx_teardown), otherwise a caller could pass a heap-allocated string and free it prematurely.
| #include "cgen/handle.h" | ||
| #include "cgen/prog/link.h" | ||
| #include "cgen/prog/map.h" | ||
|
|
There was a problem hiding this comment.
Claude: suggestion: copy_hookopts takes pointer parameters dest and src but lacks assert() precondition checks. The codebase convention (and style guide) requires assert() for pointer parameters. The same applies to copy_set further down.
| ping -c 1 -W 0.1 ${NS_IP_ADDR} | ||
| ${FROM_NS} ${BFCLI} ruleset set --from-str "chain xdp BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT rule ip4.proto icmp counter DROP" | ||
| (! ping -c 1 -W 0.1 ${NS_IP_ADDR}) | ||
| ${FROM_NS} ${BFCLI} chain get --name xdp | awk '/ip4.proto eq icmp/{getline; print $2}' | grep -q "^1$" && exit 0 || exit 1 |
There was a problem hiding this comment.
Claude: suggestion: && exit 0 || exit 1 causes the script to exit unconditionally, making the ruleset flush on line 12 unreachable dead code. Since the test harness uses set -e, the explicit exits are unnecessary. Drop them and let set -e handle failures:
| ${FROM_NS} ${BFCLI} chain get --name xdp | awk '/ip4.proto eq icmp/{getline; print $2}' | grep -q "^1$" && exit 0 || exit 1 | |
| ${FROM_NS} ${BFCLI} chain get --name xdp | awk '/ip4.proto eq icmp/{getline; print $2}' | grep -q "^1$" |
| ${FROM_NS} ${BFCLI} ruleset get | grep "^chain" | awk 'END{exit NR!=1}' | ||
| ${FROM_NS} test -e ${PINNED_PROG} | ||
|
|
||
| # Chain is still discoverable from bpffs after update |
There was a problem hiding this comment.
Claude: nit: The block under "Chain is still discoverable from bpffs after update" (lines 21-23) repeats the same two assertions from lines 18-19 with no intervening state change. Previously meaningful across a daemon restart boundary, this is now redundant.
| /// BPF token file descriptor | ||
| int token_fd; | ||
|
|
||
| int lock_fd; |
There was a problem hiding this comment.
Claude: nit: lock_fd has no inline Doxygen comment, while the adjacent token_fd does. A brief /// Lock file descriptor for bpffs pin directory would help readers. Same for cgens and stubs.
| #include <string.h> | ||
| #include <unistd.h> | ||
|
|
||
| #include <bpfilter/ctx.h> |
There was a problem hiding this comment.
Claude: nit: Uses angle brackets (<bpfilter/ctx.h>) while lines 12-19 use quotes for the same bpfilter/ headers (e.g., "bpfilter/chain.h"). Pick one style consistently within the file.
| */ | ||
|
|
||
| #include <errno.h> | ||
| #include <stdlib.h> |
There was a problem hiding this comment.
Claude: nit: <errno.h> was removed but the file still uses errno constants throughout (-ENOMEM, -EINVAL, -ENOENT, -EEXIST, -EBUSY, -ENODEV, -ENOLINK). Currently compiles via transitive inclusion through bpfilter/helper.h, but every other .c file in src/libbpfilter/ includes <errno.h> directly.
2 participants
As it is now, this PR can be merged into
main, but more polishing will be done before tagging a new release.