Repository navigation
lib: cgen: fix IPv6 fragment header size and short L4 header handling - #596
Conversation
|
@claude review |
Claude review of PR #596 (01df0e8)Both fixes are correct: the Fragment header size change matches RFC 8200 section 4.5, and the L4 dynptr slice failure path now correctly mirrors the "unsupported L4 protocol" state (r8=0, r0=NULL) instead of unconditionally accepting. BPF register usage is sound — r8 is callee-saved so the explicit zeroing after the kfunc calls is needed, and r0 must be explicitly set to 0 because Suggestions
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| /* All hooks that support BPF_PROG_TEST_RUN (excludes BF_FLAVOR_CGROUP_SOCK_ADDR). */ | ||
| static constexpr enum bf_hook kTestableHooks[] = { | ||
| BF_HOOK_XDP, |
There was a problem hiding this comment.
Claude: suggestion: kTestableHooks is an identical copy of the array in tests/e2e/verdicts/next.cpp. Consider moving it into the shared test harness (e.g., tests/harness/test.hpp) so future hook additions only need one update.
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Looks mostly good, except for a comment from Claude and one from me.
There was a problem hiding this comment.
This test should use MatcherTestsSuite, see e2e/matchers/*.cpp for examples.
Regarding fake packet creation, create relevant classes in Packet.cpp/hpp to easy creation of packets with extension headers.
79b1b13 to
9792dbd
Compare
3f37beb to
0b2e5d9
Compare
The packet builder can't create packets with IPv6 extension headers,
which are needed to test the IPv6 extension header parsers from the
matcher tests.
Add Hop-by-Hop Options and Fragment header layers. They are inserted
between the IPv6 and transport layers, and the Next Header field of
each extension header is set to the protocol of the layer following it:
Ethernet{} / IPv6{} / IPv6HopByHop{} / IPv6Fragment{} / TCP{}
Both IPv6 extension header parsers use the reserved second byte of the Fragment header (RFC 8200, section 4.5) as a length, so a non-zero value moves the L4 header offset. When the L4 header can't be read, the program accepts the packet before any rule runs. Add a packet with a non-zero Fragment reserved byte to the tcp.dport test, which uses parse_ipv6_eh.bpf.c, and to the ip6.nexthdr test, whose matcher makes bpfilter use parse_ipv6_nh.bpf.c. The Fragment header follows a Hop-by-Hop Options header because the extension header bitmask in bf_stub_parse_l3_hdr() doesn't include the Fragment header, so one right after the IPv6 header isn't parsed. Add a packet with a truncated TCP header to the ip6.saddr test: rules that don't use the L4 header must still apply to it. These tests fail until the next commit.
The Fragment header is always 8 bytes and its second byte is reserved (RFC 8200, section 4.5), but both IPv6 extension header parsers added it to the header size. If the L4 header can't be read, continue as for an unsupported L4 protocol instead of accepting the packet before any rule, so the rules and the chain policy still apply.
0b2e5d9 to
7346efd
Compare
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
LGTM, will be merged on CI passes after I rebased the branch. Thanks!
2 participants
Summary
The IPv6 Fragment header is always 8 bytes, and its second byte is reserved (RFC 8200, section 4.5). Both IPv6 extension header parsers added that byte to the header size, so a non-zero value moved the L4 header offset.
When the L4 header couldn't be read, for example because the packet ends inside it, the program returned ACCEPT before any rule ran. It now continues as it does for an unsupported L4 protocol, so the rules and the chain policy decide.
The first commit adds Hop-by-Hop Options and Fragment header layers to the test packet builder. The second adds a packet to each of the tcp.dport, ip6.nexthdr and ip6.saddr matcher tests. They fail until the third commit, which has the fix.
Related issue
None. The tests in the second commit show both cases on main (836eb74).
Testing
CMake release build (
-DNO_DOCS=1) on CentOS Stream 9, kernel 6.16.1 (x86_64), gcc 11.5, clang 22.1.3, libbpf 1.6.3, thenmake -C $BUILD test_binandctest. The tests ran in private mount, network and cgroup namespaces with their own bpffs, in place of CI's privileged container.fuzzing.parser.infiniteis not counted (it runs until stopped), andcheck.iwyudid not run (/usr/bin/iwyu_tool.pyis missing here).check.iwyu).e2e.matchers.tcp_dport,ip6_nexthdrandip6_saddrfail on all 10 hooks, each at its new packet (DROP expected, ACCEPT returned).check.iwyu).parse_ipv6_eh.bpf.cchangetcp_dportfails, without theparse_ipv6_nh.bpf.cchangeip6_nexthdrfails, and without thestub.cchangeip6_saddrfails.bfcli --verbose bpfloads chains using both extension header parsers on XDP, TC, NF and cgroup_skb hooks, so the debug print in the new block also passes the verifier.Notes for the reviewer
git bisect, so I'm happy to squash the last two or put the fix first._BF_LOW_EH_BITMASKin stub.c doesn't match the bits its comment lists. I left that for a separate PR.bf_runtime.l4_hdr, so it could be set to NULL as on the new path.AI disclosure
Claude Code drafted the tests, the packet builder change, the fix, the commit messages and this description under my direction, and ran the tests listed under Testing.
I reviewed the change and the test results.
Checklist
make -C $BUILD test_bin testpasses and the code follows the style guidecomponent: subcomponent: short description