-
Notifications
You must be signed in to change notification settings - Fork 48
lib: matcher: add ip6.traffic_class matcher #369
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.
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
lib: matcher: add ip6.traffic_class matcher #369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this task! Overall, it looks good, except for a few changes.
Some more comments:
- Rename the matcher to
ip6.dscp, so we align with nftables - Format the code properly (
make fixstyle) - Add end-to-end tests for the matcher (
tests/e2e/matchers)
src/bfcli/lexer.l
Outdated
| %s STATE_MATCHER_ICMP_TYPE | ||
| %s STATE_MATCHER_ICMP_CODE | ||
| %s STATE_MATCHER_TCP_FLAGS | ||
| %s STATE_MATCHER_IP6_TRAFFIC_CLASS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to after STATE_MATCHER_IP6_NET.
src/bpfilter/cgen/matcher/ip6.c
Outdated
| static int _bf_matcher_generate_ip6_traffic_class(struct bf_program *program, | ||
| const struct bf_matcher *matcher) | ||
| { | ||
| bf_assert(program && matcher); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(program);
assert(matcher);
src/bpfilter/cgen/matcher/ip6.c
Outdated
| { | ||
| bf_assert(program && matcher); | ||
|
|
||
| uint8_t tc = *(uint8_t *)bf_matcher_payload(matcher); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define this variable before assert().
src/bpfilter/cgen/matcher/ip6.c
Outdated
| /* Load byte 0 and extract traffic class high nibble */ | ||
| EMIT(program, BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_6, 0)); | ||
| EMIT(program, BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0x0F)); | ||
| EMIT(program, BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 4)); | ||
|
|
||
| /* Load byte 1 and extract traffic class low nibble */ | ||
| EMIT(program, BPF_LDX_MEM(BPF_B, BPF_REG_2, BPF_REG_6, 1)); | ||
| EMIT(program, BPF_ALU64_IMM(BPF_RSH, BPF_REG_2, 4)); | ||
|
|
||
| /* Combine: tc = tc_high << 4 | tc_low */ | ||
| EMIT(program, BPF_ALU64_REG(BPF_OR, BPF_REG_1, BPF_REG_2)); | ||
|
|
||
| EMIT_FIXUP_JMP_NEXT_RULE( | ||
| program, | ||
| BPF_JMP_IMM(bf_matcher_get_op(matcher) == BF_MATCHER_EQ ? BPF_JNE : | ||
| BPF_JEQ, | ||
| BPF_REG_1, tc, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be improved:
- Load 2 bytes from index 0
- Mask out irrelevant bits
& 0xff0 - Compare against
tc << 4
src/libbpfilter/matcher.c
Outdated
| static int _bf_parse_traffic_class(enum bf_matcher_type type, | ||
| enum bf_matcher_op op, void *payload, | ||
| const char *raw_payload) | ||
| { | ||
| bf_assert(payload && raw_payload); | ||
|
|
||
| unsigned long tc; | ||
| char *endptr; | ||
|
|
||
| tc = strtoul(raw_payload, &endptr, BF_BASE_10); | ||
| if (*endptr == '\0' && tc <= UINT8_MAX) { | ||
| *(uint8_t *)payload = (uint8_t)tc; | ||
| return 0; | ||
| } | ||
|
|
||
| tc = strtoul(raw_payload, &endptr, BF_BASE_16); | ||
| if (*endptr == '\0' && tc <= UINT8_MAX) { | ||
| *(uint8_t *)payload = (uint8_t)tc; | ||
| return 0; | ||
| } | ||
|
|
||
| bf_err( | ||
| "\"%s %s\" expects a decimal or hexadecimal traffic class value (0-255), not '%s'", | ||
| bf_matcher_type_to_str(type), bf_matcher_op_to_str(op), raw_payload); | ||
|
|
||
| return -EINVAL; | ||
| } | ||
|
|
||
| static void _bf_print_traffic_class(const void *payload) | ||
| { | ||
| bf_assert(payload); | ||
|
|
||
| (void)fprintf(stdout, "0x%02" PRIx8, *(uint8_t *)payload); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worth creating more generic parsing/printing functions (i.e. _bf_parse_u8) that we could reuse later on (e.g. instead of _bf_parse_icmp_code).
tests/unit/libbpfilter/matcher.c
Outdated
| bf_matcher_dump(matcher, &prefix); | ||
| } | ||
|
|
||
| static void ip6_traffic_class(void **state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test for unsupported values, extreme values, and add a test for the print function.
Address review feedback for PR facebook#369: - Rename BF_MATCHER_IP6_TRAFFIC_CLASS to BF_MATCHER_IP6_DSCP - Rename ip6.traffic_class to ip6.dscp in lexer and strings - Move lexer state after STATE_MATCHER_IP6_NET - Simplify BPF code: load 2 bytes from offset 0, mask with 0x0ff0, compare against dscp << 4 - Split bf_assert(program && matcher) into separate assert() calls - Use generic _bf_parse_u8/_bf_print_u8 functions - Add comprehensive unit tests (hex input, error cases, extreme values) - Add e2e test file tests/e2e/matchers/ip6_dscp.sh
|
Thanks for the review! I've addressed all the feedback:
Ready for re-review! |
src/bpfilter/cgen/program.c
Outdated
| case BF_MATCHER_IP6_DADDR: | ||
| case BF_MATCHER_IP6_DNET: | ||
| case BF_MATCHER_IP6_NEXTHDR: | ||
| case BF_MATCHER_IP6_TRAFFIC_CLASS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to rename this.
|
Similarly to #364 all these changes should be in a single commit. |
d50339d to
bd23ec8
Compare
|
Updated:
Ready for re-review! |
qdeslandes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except for this last change. You will also need to rebase on main.
| @@ -0,0 +1,40 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file must be added to tests/e2e/CMakeLists.txt.
bd23ec8 to
629c9b3
Compare
|
Rebased on main and added ip6_dscp.sh to CMakeLists.txt. |
qdeslandes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last change, otherwise LGTM!
src/libbpfilter/matcher.c
Outdated
| assert(payload); | ||
| assert(raw_payload); | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the extra line.
Add new ip6.dscp matcher to filter packets by IPv6 DSCP field. Supports eq and not operators with decimal (0-255) or hex (0x00-0xff) values. Changes: - Add BF_MATCHER_IP6_DSCP enum in matcher.h - Add generic _bf_parse_u8/_bf_print_u8 functions in matcher.c - Add simplified BPF code in ip6.c (load 2 bytes, mask 0x0ff0, compare dscp<<4) - Add switch case in program.c - Add CLI parsing in lexer.l (state after IP6_NET) - Add e2e tests (tests/e2e/matchers/ip6_dscp.sh) - Add comprehensive unit tests Example: rule ip6.dscp eq 46 ACCEPT Closes facebook#361
629c9b3 to
864f807
Compare
qdeslandes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Summary
ip6.traffic_classmatcher to filter packets by IPv6 Traffic Class fieldeqandnotoperatorsExample usage
Changes
matcher.h: AddedBF_MATCHER_IP6_TRAFFIC_CLASSenummatcher.c: Added parse/print functions and metadataip6.c: Added BPF code generation (byte loads for portability)program.c: Added switch case routinglexer.l: Added CLI parsing supporttests/: Added unit testsCloses #361