lib: log: support packet 5-tuple logging - #578
Conversation
|
|
||
| - ``log $HEADERS``: log specific packet headers. ``$HEADERS`` is a comma-separated list of ``link`` (layer 2), ``internet`` (layer 3), and/or ``transport`` (layer 4). Only supported by packet-based hooks (XDP, TC, NF, cgroup_skb). | ||
| - ``log 5-tuple``: log source and destination addresses and ports, and the transport protocol. This mode is only supported by packet-based hooks and only emits entries for IPv4/IPv6 packets using TCP or UDP. It is mutually exclusive with the packet-layer options; unsupported packets do not fall back to packet-layer logging. | ||
| - ``log``: log all available data for the hook type. For packet-based hooks, this is equivalent to ``log link,internet,transport``. For ``BF_HOOK_CGROUP_SOCK_ADDR_*`` hooks, this records the process ID, process name, destination address, and destination port. Sendmsg hooks additionally include the source address. |
There was a problem hiding this comment.
"unsupported packets do not fall back to packet-layer logging"
-> "unsupported packets are not logged."
There was a problem hiding this comment.
alright, will simplify the wording
| } | ||
| } | ||
|
|
||
| 5-tuple { yylval.sval = strdup(yytext); return STRING; } |
There was a problem hiding this comment.
Don't create a specific token for the 5-tuple keyword, update the definition of STRING below to support - in strings instead.
There was a problem hiding this comment.
OH Okay, will do that
|
|
||
| inet_ntop(family, log->pkt_5_tuple.saddr, src_addr, sizeof(src_addr)); | ||
| inet_ntop(family, log->pkt_5_tuple.daddr, dst_addr, sizeof(dst_addr)); | ||
| protocol = bf_ipproto_to_str(log->l4_proto); |
There was a problem hiding this comment.
Add a comment that this only works because l4_proto is either TCP or UDP, and those are defined in bf_ipproto_to_str.
There was a problem hiding this comment.
Gotcha, will add comment for improved documentation and understanding.
| "link,internet" | ||
| "internet,transport" | ||
| "link,internet,transport" | ||
| "log 5-tuple" |
| rule->disabled = r; | ||
| } | ||
|
|
||
| if (rule->log != BF_LOG_OPT_DEFAULT && (rule->log & tuple) && |
There was a problem hiding this comment.
BF_LOG_DEFAULT should represent link,internet,transport logs.
There was a problem hiding this comment.
Will, correct this masking situation
| struct LogCapture | ||
| { | ||
| std::vector<struct bf_log> entries; | ||
| }; | ||
|
|
||
| static int captureLog(void *ctx, void *data, size_t size) | ||
| { | ||
| auto *capture = static_cast<LogCapture *>(ctx); | ||
| struct bf_log log = {}; | ||
|
|
||
| assert_int_equal(sizeof(log), size); | ||
| std::memcpy(&log, data, sizeof(log)); | ||
| capture->entries.push_back(log); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| static void assertAddress(const uint8_t *actual, int family, | ||
| const char *expected) | ||
| { | ||
| std::array<uint8_t, sizeof(struct in6_addr)> addr = {}; | ||
| size_t len = family == AF_INET ? sizeof(struct in_addr) : sizeof(addr); | ||
|
|
||
| assert_int_equal(1, inet_pton(family, expected, addr.data())); | ||
| assert_memory_equal(addr.data(), actual, len); | ||
| } | ||
|
|
||
| static void assertTuple(const struct bf_log &log, uint16_t l3Proto, | ||
| uint8_t l4Proto, const char *saddr, const char *daddr, | ||
| uint16_t sport, uint16_t dport) | ||
| { | ||
| int family = l3Proto == ETH_P_IP ? AF_INET : AF_INET6; | ||
|
|
||
| assert_int_equal(BF_LOG_TYPE_PACKET_5_TUPLE, log.log_type); | ||
| assert_int_equal(l3Proto, log.l3_proto); | ||
| assert_int_equal(l4Proto, log.l4_proto); | ||
| assert_int_equal(0, log.rule_id); | ||
| assert_int_equal(BF_VERDICT_DROP, log.verdict); | ||
| assertAddress(log.pkt_5_tuple.saddr, family, saddr); | ||
| assertAddress(log.pkt_5_tuple.daddr, family, daddr); | ||
| assert_int_equal(sport, log.pkt_5_tuple.sport); | ||
| assert_int_equal(dport, log.pkt_5_tuple.dport); | ||
| } |
There was a problem hiding this comment.
Those are generic functions that should be part of the testing harness instead.
There was a problem hiding this comment.
Hmmm makes sense will do that
| // A 5-tuple is only complete for IPv4/IPv6 packets using TCP/UDP. | ||
| // Skip only the log action for other packets, leaving the rule's | ||
| // remaining actions and verdict unchanged. |
There was a problem hiding this comment.
Use /* */ for multiline comments.
| // Rate-limited log: check last_log_ts in the state map before logging. | ||
| // | ||
| // R9 (callee-saved) holds the pointer to this rule's state entry | ||
| // across the bpf_ktime_get_ns() call. |
There was a problem hiding this comment.
Use /* */ for multiline comments.
| // Outer skip: state_map is NULL (shouldn't happen at runtime, | ||
| // but the verifier requires the NULL check). |
There was a problem hiding this comment.
Use /* */ for multiline comments.
| if (rule->log == BF_FLAG(BF_LOG_OPT_5_TUPLE)) { | ||
| // A 5-tuple is only complete for IPv4/IPv6 packets using TCP/UDP. | ||
| // Skip only the log action for other packets, leaving the rule's | ||
| // remaining actions and verdict unchanged. | ||
| EMIT(program, BPF_JMP_IMM(BPF_JEQ, BPF_REG_7, htobe16(ETH_P_IP), 2)); | ||
| EMIT(program, BPF_JMP_IMM(BPF_JEQ, BPF_REG_7, htobe16(ETH_P_IPV6), 1)); | ||
| { | ||
| // Outer skip: state_map is NULL (shouldn't happen at runtime, | ||
| // but the verifier requires the NULL check). | ||
| _clean_bf_jmpctx_ struct bf_jmpctx null_ctx = | ||
| bf_jmpctx_get(program, BPF_JMP_IMM(BPF_JEQ, BPF_REG_9, 0, 0)); | ||
|
|
||
| if (rule->index > 0) { | ||
| EMIT(program, | ||
| BPF_ALU64_IMM( | ||
| BPF_ADD, BPF_REG_9, | ||
| (int)(rule->index * sizeof(struct bf_rule_state)))); | ||
| } | ||
|
|
||
| EMIT(program, BPF_EMIT_CALL(BPF_FUNC_ktime_get_ns)); | ||
|
|
||
| EMIT(program, BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_9, 0)); | ||
| EMIT(program, BPF_MOV64_REG(BPF_REG_2, BPF_REG_0)); | ||
| EMIT(program, BPF_ALU64_REG(BPF_SUB, BPF_REG_2, BPF_REG_1)); | ||
| _clean_bf_jmpctx_ struct bf_jmpctx l3_ctx = | ||
| bf_jmpctx_get(program, BPF_JMP_A(0)); | ||
|
|
||
| EMIT(program, BPF_JMP_IMM(BPF_JEQ, BPF_REG_8, IPPROTO_TCP, 2)); | ||
| EMIT(program, BPF_JMP_IMM(BPF_JEQ, BPF_REG_8, IPPROTO_UDP, 1)); | ||
| { | ||
| // Load log_rate_ns as a 64-bit immediate into R1. | ||
| const struct bpf_insn rate_insn[2] = { | ||
| BPF_LD_IMM64(BPF_REG_1, rule->log_rate_ns), | ||
| }; | ||
| EMIT(program, rate_insn[0]); | ||
| EMIT(program, rate_insn[1]); | ||
| } | ||
|
|
||
| { | ||
| // Inner skip: delta < log_rate_ns means still within window. | ||
| _clean_bf_jmpctx_ struct bf_jmpctx rate_ctx = bf_jmpctx_get( | ||
| program, BPF_JMP_REG(BPF_JLT, BPF_REG_2, BPF_REG_1, 0)); | ||
|
|
||
| EMIT(program, BPF_STX_MEM(BPF_DW, BPF_REG_9, BPF_REG_0, 0)); | ||
| _clean_bf_jmpctx_ struct bf_jmpctx l4_ctx = | ||
| bf_jmpctx_get(program, BPF_JMP_A(0)); | ||
|
|
||
| r = program->runtime.ops->gen_inline_log(program, rule); | ||
| r = _bf_program_generate_log(program, rule); | ||
| if (r) | ||
| return r; | ||
| } | ||
| } | ||
| } else if (rule->log) { | ||
| r = program->runtime.ops->gen_inline_log(program, rule); | ||
| } else { | ||
| r = _bf_program_generate_log(program, rule); | ||
| if (r) | ||
| return r; | ||
| } |
There was a problem hiding this comment.
This could be simplified:
if log:
if log 5-tuple:
setup 5-tuple logging
bf_program_generate_log();
In bf_program_generate_log:
if log_rate_ns:
rate-limit setup
gen_inline_log();
There was a problem hiding this comment.
Agreed. I will keep tuple eligibility setup separate and then call that common logging generator once.
|
Thanks a lot for your detailed review. @qdeslandes |
|
@qdeslandes , can you rerun this CI again. |
|
@qdeslandes Could you review this pr. |
|
@claude review |
Claude review of PR #578 (acc0dcd)Suggestions
Nits
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| [a-zA-Z0-9_]+ { yylval.sval = strdup(yytext); return STRING; } | ||
| [a-zA-Z0-9_-]+ { yylval.sval = strdup(yytext); return STRING; } |
There was a problem hiding this comment.
Claude: suggestion: The pattern change from [a-zA-Z0-9_]+ to [a-zA-Z0-9_-]+ allows hyphens in all STRING tokens, but only 5-tuple needs it. Under flex longest-match rules, input like -foo (no leading space) becomes a single STRING rather than two tokens (- + foo). In practice this is low risk since the grammar expects whitespace between tokens, but a more targeted approach would be safer — e.g., matching 5-tuple as a dedicated keyword before the catch-all, or restricting hyphens to non-leading positions: [a-zA-Z0-9_][a-zA-Z0-9_-]*.
There was a problem hiding this comment.
I think this is probably fine as is. The weird part is that -- and - become valid set and chain names, but __ and _ already were.
| }; | ||
|
|
||
| /** Log all available packet headers. */ | ||
| #define BF_LOG_DEFAULT \ |
There was a problem hiding this comment.
Claude: nit: BF_LOG_DEFAULT manually writes (1ULL << X) | ... while the project provides BF_FLAG() and BF_FLAGS() in helper.h for exactly this pattern (used extensively in matcher.c, hook.c, opts.c, set.c). Consider:
#define BF_LOG_DEFAULT BF_FLAGS(BF_LOG_OPT_LINK, BF_LOG_OPT_INTERNET, BF_LOG_OPT_TRANSPORT)
|
I'll go over claude's comments (to check their accuracy) and give this a review sometime in the next few days |
Alrighty! |
yaakov-stein
left a comment
yaakov-stein
left a comment
There was a problem hiding this comment.
Overall looks pretty good, left a few comments and please squash the follow-up fix commit into the original commit so the change lands as one self-contained commit.
| BF_LOG_OPT_5_TUPLE, | ||
|
|
||
| /** Log all available data for the hook type. */ | ||
| BF_LOG_OPT_DEFAULT = 0xFF, |
There was a problem hiding this comment.
At the moment, I don't believe we can remove this. While the other options (BF_LOG_OPT_(LINK|INTERNET|TRANSPORT)) are used for their specific bit (and thus _BF_LOG_OPT_MAX < 8 as it is a uint8_t), BF_LOG_OPT_DEFAULT is a sentinel meant to signify "log everything" in both the packet and CGROUP_SOCK_ADDR context (and anything else we may add in the future). This value was chosen because it is by definition both different than any other value (it's not a single bit, it's all the bits) and also will match every bit of the other values when used as a bit mask.
Therefore, if we remove this, we can no longer differentiate between log link,internet,transport and log, and thus 1) log link,internet,transport becomes valid syntax for CGROUP_SOCK_ADDR, which doesn't make much sense, and 2) not as much of a problem, but we can't print what the user gave as input for packet logging.
Check out #517 for a bit more background if you're interested.
There was a problem hiding this comment.
Oh I see, will be checking in that pr, I really appreciate your detailed reasoning.
I missed that BF_LOG_OPT_DEFAULT is intentionally a semantic sentinel for bare log, rather than only a packet-header mask.
I will restore the sentinel, retain BF_LOG_DEFAULT for the explicit link,internet,transport mask, and preserve that distinction through validation, CLI printing, and packet codegen.
There was a problem hiding this comment.
I think instead of keeping BF_LOG_DEFAULT (which is confusing with BF_LOG_OPT_DEFAULT), let's rename it to something like BF_LOG_PACKET_HEADERS or something like that.
There was a problem hiding this comment.
Okayy, will do that.
| } | ||
|
|
||
| [a-zA-Z0-9_]+ { yylval.sval = strdup(yytext); return STRING; } | ||
| [a-zA-Z0-9_-]+ { yylval.sval = strdup(yytext); return STRING; } |
There was a problem hiding this comment.
I think this is probably fine as is. The weird part is that -- and - become valid set and chain names, but __ and _ already were.
| return 0; | ||
| } | ||
|
|
||
| static int _bf_program_generate_log(struct bf_program *program, |
There was a problem hiding this comment.
I think once we are making a _bf_program_generate_log method, let's put all of the logging logic there and can keep it simple doing something like this:
static int _bf_program_generate_log(struct bf_program *program,
const struct bf_rule *rule)
{
// Declare and assert
if (!rule->log)
return 0;
if (rule->log == BF_FLAG(BF_LOG_OPT_5_TUPLE)) {
...
}
if (rule->log_rate_ns) {
...
}
return program->runtime.ops->gen_inline_log(program, rule);
}
There was a problem hiding this comment.
Alrighty, I will move the no-log early return, 5-tuple eligibility guards, rate-limit handling, and final gen_inline_log() call into _bf_program_generate_log().
so, _bf_program_generate_rule() will then call this helper once, while unsupported tuple packets will still continue through counters, marks, and verdict generation.
| } | ||
|
|
||
| if (rule->log && rule->log != BF_LOG_OPT_DEFAULT && | ||
| if ((rule->log & tuple) && (rule->log & tuple) != rule->log) { |
There was a problem hiding this comment.
Per this comment, we'll need to switch this to:
if (rule->log != BF_LOG_OPT_DEFAULT &&
(rule->log & tuple) && rule->log != tuple) {
There was a problem hiding this comment.
Oh Cool, I will use the suggested condition.
acc0dcd to
6884e58
Compare
|
Thanks for the review @yaakov-stein. I’ve addressed the requested changes. |
6884e58 to
f123303
Compare
4 participants
Summary
Add
log 5-tupleas an exclusive packet logging mode.The implementation introduces a dedicated ELF stub that emits compact records
containing source and destination addresses, source and destination ports, and
the transport protocol. It supports complete IPv4/IPv6 TCP/UDP tuples while
preserving the existing fixed-size
struct bf_logreservation and ring-buffercapacity.
Unsupported packets do not emit a tuple record or fall back to raw-header
logging. Their counters, marks, verdicts, and normal rule execution remain
unchanged.
Related issue
Fixes #568
Testing
Added coverage for:
log 5-tuplesyntax, with and withouteverycgroup_sock_addrDebug build with sanitizers and coverage enabled:
Release build:
Additional
passedverification:make -C build-issue568fixstylemake -C build-issue568 test_bintestmake -C build-issue568 docNotes for the reviewer
5-tupleis mutually exclusive withlink,internet, andtransport.Tuple records are emitted only for IPv4/IPv6 packets using TCP or UDP, following
the eligibility behaviour discussed in #568. The eligibility check happens
before rate-limit bookkeeping, so an unsupported packet does not consume the
rule's
everyinterval.cgroup_sock_addris intentionally unsupported for this mode. Existing rawpacket logging and socket-address logging remain unchanged.
The new log type and ELF-stub identifiers are appended so existing identifier
values remain stable.
AI disclosure
I used AI for codebase exploration, understanding the existing logging and BPF
code-generation paths, design iteration, parts of the implementation and tests,
verification, review, and wording of this PR.
I reviewed the resulting design and test evidence and remain responsible for
the submitted changes.
Checklist
make -C $BUILD test_bin testpasses and the code follows the style guidecomponent: subcomponent: short description