Repository navigation
lib: cgen: keep the log ring buffer across set updates - #605
prashantpcm94 wants to merge 1 commit into
Conversation
|
@claude review |
Claude review of PR #605 (29f52b4)Suggestions
Nits
Errors
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| assert_int_equal(1, capture.entries.size()); | ||
| assert_int_equal(BF_VERDICT_DROP, capture.entries[0].verdict); | ||
|
|
||
| new_fd = bf_chain_logs_fd("test_log_update"); |
There was a problem hiding this comment.
Claude: suggestion: new_fd is only checked with >= 0. That check also passes without this fix, because the updated chain logs and the new handle had its own bf_lmap before this PR too. So it doesn't show that bf_chain_logs_fd() now returns the same ring buffer. Could the test compare the map IDs of fd and new_fd with bpf_map_get_info_by_fd() (libbpf 1.2+)? Another option is to open a second ring_buffer on new_fd and check that the same entry shows up there. If neither is wanted, the new_fd lines could be dropped.
| * | ||
| * The file descriptor keeps receiving the chain's logs after | ||
| * `bf_chain_update()` and `bf_chain_update_set()`, until the chain is | ||
| * replaced with `bf_chain_set()` or flushed. |
There was a problem hiding this comment.
Claude: suggestion: Two points on this paragraph:
- "until the chain is replaced with
bf_chain_set()or flushed" leaves outbf_ruleset_set(), which also replaces every chain (_bf_ruleset_flush()followed bybf_cgen_new()), so a reader's FD goes stale there too. "Flushed" could meanbf_chain_flush()orbf_ruleset_flush(). Maybe list them all, e.g. "until the chain is replaced (bf_chain_set(),bf_ruleset_set()) or removed (bf_chain_flush(),bf_ruleset_flush())". - The return value of
bf_chain_logs_fd()changes. Before this PR, after an update that left no logging rule (or a set update that disabled them), the chain had nolmapand the call returned-ENOENT("chain '%s' has no logs buffer"). Now it succeeds and returns an FD to a ring buffer that nothing writes to, sobfcli chain logspolls with no output. Whether the call fails now depends on the chain's history, not on what it currently contains. Could the doc say so, e.g. "a chain that logged keeps its logs buffer, even after an update that leaves no logging rule"? The commit body could also mention that the ring buffer stays allocated and pinned for as long as the chain exists.
| // Emptying the set disables the only logging rule, refilling it enables | ||
| // the rule again | ||
| assert_int_equal(0, | ||
| bf_chain_update_set("test_log_empty_set", none.get().get(), |
There was a problem hiding this comment.
Claude: suggestion: The intermediate state is never run: the set is empty, BF_CHAIN_LOG is cleared, and lmap is kept although the program doesn't log. Could a bft_assert_prog_run() with dport 53 expecting bft_hook_accept(BF_HOOK_XDP), followed by assert_int_equal(0, ring_buffer__consume(rb)), go between the two bf_chain_update_set() calls? That checks that the disabled rule really emits nothing. A bf_chain_logs_fd() call in that state would also record the behaviour change, since it now succeeds where it used to return -ENOENT.
| * Prior to loading the BPF program, multiple BPF maps are created to store | ||
| * the counters, the debug strings, and the sets. If the program can't be | ||
| * loaded, all the maps are destroyed. | ||
| * loaded, all the maps are destroyed. If `prog->handle` already has a log |
There was a problem hiding this comment.
Claude: suggestion: With this change, the sentence just before the new one ("If the program can't be loaded, all the maps are destroyed.") no longer holds for the log map. When prog->handle->lmap was duplicated from the old handle, freeing the new handle only closes the duplicated FD, and the BPF map stays alive through the old handle. That is what this PR wants, so could the paragraph say it, e.g. "the maps created by this function are destroyed; a reused log map is only closed"?
| /** | ||
| * @brief Get the file descriptor of a chain's logs buffer. | ||
| * | ||
| * The file descriptor keeps receiving the chain's logs after |
There was a problem hiding this comment.
Claude: suggestion: The user-facing docs don't mention the new behaviour yet. In doc/usage/explanation/lifecycle.rst, "Replacing a chain" lists what chain update keeps and what it resets, and "Updating a set" says chain update-set preserves the counters. Should they also say that a running bfcli chain logs keeps printing entries across chain update and chain update-set, but has to be restarted after chain set, ruleset set, or a flush? A short note in the chain logs section of doc/usage/reference/bfcli.rst would work too. Those files aren't part of this diff, which is why this comment is attached here.
| * On success, the new program is stored in the codegen, and the previous | ||
| * program is unloaded and freed. | ||
| * program is unloaded and freed. The log ring buffer is kept, so file | ||
| * descriptors returned by `bf_chain_logs_fd()` stay valid. |
There was a problem hiding this comment.
Claude: nit: "so file descriptors returned by bf_chain_logs_fd() stay valid": those FDs were already valid before this change, because they hold their own reference to the old map. The regression was that nothing wrote to that map anymore. Could this use the wording from the cgen.c comment and bpfilter.h, "keep receiving the chain's logs"?
|
|
||
| (void)state; | ||
|
|
||
| assert_int_equal(0, bf_chain_set(chain.get().get(), nullptr)); |
There was a problem hiding this comment.
Claude: nit: The other tests in this file, including the two new set tests, install the chain with BFT_CHAIN_SET(...). chain isn't used after this call, so could it be built inline in BFT_CHAIN_SET(bf::Chain(...) << bf::Rule(...)) to match? updated still needs to be a variable for bf_chain_update().
| _cleanup_close_ int fd = -1; | ||
| struct bft_log_capture capture; | ||
| struct ring_buffer *rb; | ||
| auto set = bf::Set({BF_MATCHER_UDP_DPORT}, "ports"); |
There was a problem hiding this comment.
Claude: nit: This file now uses bf::Set, but it doesn't include "Set.hpp"; it builds only because Chain.hpp includes it. The other e2e files that use bf::Set (tests/e2e/matchers/udp_dport.cpp, icmp_type.cpp, icmpv6_type.cpp, ...) include "Set.hpp" directly. Could it be added next to "Rule.hpp"?
| fd = bf_chain_logs_fd("test_log_empty_set"); | ||
| assert_true(fd >= 0); | ||
| rb = ring_buffer__new(fd, bft_capture_log, &capture, nullptr); | ||
| assert_non_null(rb); |
There was a problem hiding this comment.
Claude: nit: This comment takes two lines, and doc/developers/style.rst says: "Use // for comments that fit on one line, /* */ for longer comments". tupleLogging() in this file already follows that rule. Could it be /* ... */?
566f124 to
29f52b4
Compare
|
@claude review |
| Print a chain's log entries. | ||
|
|
||
| bfcli will print log entries as they are published by the chain. Hit ``Ctrl+C`` to quit. | ||
| bfcli will print log entries as they are published by the chain. Hit ``Ctrl+C`` to quit. The log entries keep coming after ``chain update`` and ``chain update-set``. After ``chain set``, ``ruleset set``, or a flush, start ``chain logs`` again. |
There was a problem hiding this comment.
Claude: nit: Now that the ring buffer outlives the program, a reader can still get entries that the old program published before the switch but that are only consumed after chain update. struct bf_log describes itself (log_type, l3_proto, l4_proto) and the hook can't change, so these entries still decode correctly. rule_id is different: it's an index into the chain version that emitted the entry. chain update can reorder, add or remove rules, so a stale entry can print an index that now points to another rule. Could the docs say that each entry's rule index refers to the chain version that published it? This could go here, and maybe in the bf_chain_logs_fd() doc.
| Print a chain's log entries. | ||
|
|
||
| bfcli will print log entries as they are published by the chain. Hit ``Ctrl+C`` to quit. | ||
| bfcli will print log entries as they are published by the chain. Hit ``Ctrl+C`` to quit. The log entries keep coming after ``chain update`` and ``chain update-set``. After ``chain set``, ``ruleset set``, or a flush, start ``chain logs`` again. |
There was a problem hiding this comment.
Claude: nit: Two points on "After chain set, ruleset set, or a flush, start chain logs again.":
- After a flush the chain no longer exists. A new
chain logsonly works once the chain has been set again; until then it fails with "has no logs buffer". - A running
chain logsdoesn't exit in any of these cases. It keeps polling the orphaned ring buffer and goes quiet.
Could the doc say that, and name chain flush / ruleset flush explicitly, as bpfilter.h does?
| return info.id; | ||
| } | ||
|
|
||
| static void logsFdSurvivesChainUpdate(void **state) |
There was a problem hiding this comment.
Claude: nit: The three new tests use BF_LOG_OPT_DEFAULT and check how long the logs buffer lives across bf_chain_update() / bf_chain_update_set(), not 5-tuple logging. You offered in the PR description to move them, and a dedicated file would make them easier to find. For example, tests/e2e/actions/log.cpp could be registered with bf_add_e2e_c_test() next to actions/log_5_tuple.cpp in tests/e2e/CMakeLists.txt. log_5_tuple.cpp would then stay scoped to the 5-tuple format.
| public: | ||
| Set(std::vector<bf_matcher_type> key): | ||
| _key {std::move(key)} | ||
| Set(std::vector<bf_matcher_type> key, std::string name = {}): |
There was a problem hiding this comment.
Claude: nit: Could the name be a std::optional<std::string> that defaults to std::nullopt? Rule.hpp handles its optional arguments that way (counters, logRateNs). With the empty-string sentinel, a test can't build a set named "", although bf_set_new() accepts any non-NULL name. With std::optional, "no name" and "empty name" stay distinct, and get() becomes _name ? _name->c_str() : nullptr.
29f52b4 to
4417a42
Compare
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Test details aren't relevant in the commit message.
It's worth documenting in the code that if the updated chain doesn't log at all, the ring buffer will remain. I'm not sure what to think about that, it's a stale resource that might or might not be used again.
For rules disabled by bpfilter, it makes sense to keep the log buffer, as the rule is technically still there. However, if the chain changes so that no rule logs anymore, having the log buffer around doesn't make sense.
6f27e70 to
8ec4b7f
Compare
|
Thanks, changed as suggested:
|
|
Ping! |
8ec4b7f to
2ea0d80
Compare
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Few nits, also CI is failing.
| static bool _bf_cgen_has_log_rule(const struct bf_chain *chain) | ||
| { | ||
| bf_list_foreach (&chain->rules, rule_node) { | ||
| const struct bf_rule *rule = bf_list_node_get_data(rule_node); | ||
|
|
||
| if (rule->log) | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
chain->flags will have BF_CHAIN_LOG bit set if the chain is expected to log.
There was a problem hiding this comment.
As discussed, the log ring buffer is now only kept across bf_chain_update_set(), which doesn't change the rules, so this helper is gone.
| static uint32_t mapId(int fd) | ||
| { | ||
| struct bpf_map_info info = {}; | ||
| uint32_t len = sizeof(info); | ||
|
|
||
| assert_ok(bpf_map_get_info_by_fd(fd, &info, &len)); | ||
|
|
||
| return info.id; | ||
| } |
There was a problem hiding this comment.
This could be a helper a the harness level.
There was a problem hiding this comment.
Done, it's bft_map_id() in the harness.
| assert_fd(fd); | ||
| rb = ring_buffer__new(fd, bft_capture_log, &capture, nullptr); | ||
| assert_non_null(rb); | ||
|
|
There was a problem hiding this comment.
assert_fd(fd = bf_chain_logs_fd("test_log_update_set"));
assert_non_null(rb = ring_buffer__new(fd, bft_capture_log, &capture, nullptr));
2ea0d80 to
66b89a9
Compare
|
Reworked as discussed: the log ring buffer is kept across |
bf_chain_update_set() rebuilds the chain's program through bf_cgen_update(), and the new program created its own log ring buffer. A file descriptor returned by bf_chain_logs_fd() before the update still pointed at the old ring buffer, which nothing writes to anymore, so its reader silently stopped getting logs. Reuse the old program's log map when bf_chain_update_set() updates the chain. bf_chain_update() replaces the chain's rules, so it still creates a new log ring buffer, and callers get a new file descriptor from bf_chain_logs_fd().
66b89a9 to
62ef76b
Compare
Summary
bf_chain_logs_fd()returns a file descriptor for the chain's log ring buffer.bf_chain_update_set()rebuilds the chain's program throughbf_cgen_update(), and the new program created a new log ring buffer. A file descriptor opened before the update still pointed at the old ring buffer, which nothing writes to anymore, so a long-running reader (bfcli chain logs, or a service that opens the FD once) silently stopped getting logs after every set update.This reuses the old program's log map when
bf_chain_update_set()updates the chain (newBF_CGEN_UPDATE_PRESERVE_LOGSflag, next toBF_CGEN_UPDATE_PRESERVE_COUNTERS).bf_chain_update()replaces the chain's rules, so it still creates a new log ring buffer: callers get a new file descriptor frombf_chain_logs_fd(). Thebf_chain_logs_fd()documentation and the user docs (explanation/lifecycle.rst,chain logsinreference/bfcli.rst) say so.Related issue
None.
Testing
CMake release build (
-DWITH_DOCS=OFF -DWITH_TESTS_BENCHMARKS=OFF) on CentOS Stream 9, kernel 6.16.1, gcc 11.5, clang 22.1.3, libbpf 1.6.3, thenctest -j16(withoutfuzzing.parser.infinite, which runs until stopped):check.iwyudidn't run becauseiwyu_tool.pyisn't installed here.make -C $BUILD doc(with-DWITH_DOCS=ON) builds with no warnings, as on main.e2e.actions.log(new,tests/e2e/actions/log.cpp):logsFdSurvivesSetUpdate: opens the logs file descriptor, adds a port to the set withbf_chain_update_set(), sends a packet to that port, and expects one log entry.logsFdSurvivesEmptySet: emptying the set disables the only logging rule;bf_chain_logs_fd()still returns the same ring buffer (same map ID), and refilling the set logs to the file descriptor opened at the start.Without the
cgenchange both fail (no entry on the old file descriptor; no logs buffer once the set is empty). With it they pass 10 runs in a row.AI disclosure
Claude Code found the issue, drafted the change, the tests, the commit message and this description under my direction, and ran the tests listed under Testing. I reviewed the change and the results.
Checklist
make -C $BUILD testpasses (check.iwyunot run, see Testing) and the code follows the style guidecomponent: subcomponent: short description