Skip to content

refactor(ebpf/maps_libbpf): drop ConnKey::new IpAddr ctor + both unreachable!() arms#309

Open
0xghost42 wants to merge 1 commit into
domcyrus:mainfrom
0xghost42:refactor/306-connkey-typesafe-ctors
Open

refactor(ebpf/maps_libbpf): drop ConnKey::new IpAddr ctor + both unreachable!() arms#309
0xghost42 wants to merge 1 commit into
domcyrus:mainfrom
0xghost42:refactor/306-connkey-typesafe-ctors

Conversation

@0xghost42
Copy link
Copy Markdown
Contributor

Summary

ConnKey::new and ConnKey::new_icmp both accepted IpAddr, matched (src_ip, dst_ip) and panicked on the mixed-family arm via unreachable!("Mixed IPv4/IPv6 addresses not supported"). In practice every in-repo caller of new already went through the typed new_v4(Ipv4Addr, Ipv4Addr, ...) / new_v6(Ipv6Addr, Ipv6Addr, ...) wrappers, so the mixed arm in new was unreachable by construction — it existed only because the inner constructor lost the family information by widening to IpAddr. Same shape as the recently merged stun fix in #289.

This goes with option 2 from #306 (you noted you would defer to my call): drop the IpAddr entry points entirely so the typed surface is the only way in.

What changed

src/network/platform/linux/ebpf/maps_libbpf.rs:

  • Replace pub fn new(IpAddr, IpAddr, ...) with two small private helpers empty_v4 / empty_v6 (build a zeroed key with the right family and proto) plus fill_v4 / fill_v6 (write the address slots in the family-specific endianness). new_v4 / new_v6 call these directly — there is no longer a shared IpAddr-typed seam.
  • Replace pub fn new_icmp(IpAddr, IpAddr, u16) with concrete new_icmp_v4(Ipv4Addr, Ipv4Addr, u16) and new_icmp_v6(Ipv6Addr, Ipv6Addr, u16), built on the same helpers.
  • Pull the family numbers and IPPROTO_* byte values out as named constants (AF_INET = 2, AF_INET6 = 10, IPPROTO_TCP = 6, IPPROTO_UDP = 17, IPPROTO_ICMP = 1, IPPROTO_ICMPV6 = 58) so the constructors read at the level the kernel headers use.

src/network/platform/linux/ebpf/tracker_libbpf.rs:

  • lookup_icmp now dispatches on (src_ip, dst_ip) at the entry point (matching the shape lookup already used for the TCP/UDP path) and forwards to private lookup_icmp_v4 / lookup_icmp_v6 helpers, which pass concrete address types into the new new_icmp_v4 / new_icmp_v6 constructors.
  • The mixed-family case is now handled exactly like the TCP/UDP path: a log::warn! plus None return, no unreachable!() panic.

Net effect

  • Both unreachable!() arms are gone.
  • The wire-format byte layout of ConnKey is unchanged.
  • The public surface (new_v4 / new_v6 / new new_icmp_v4 / new new_icmp_v6) is now typesafe — passing mismatched address families is a compile error rather than a runtime panic.

Tests

Five new #[cfg(test)] mod tests cases in maps_libbpf.rs lock the byte layout of new_v4 / new_v6 / new_icmp_v4 / new_icmp_v6 against the previous behaviour:

  • new_v4_writes_little_endian_addrs_and_tcp_proto — full IPv4 key shape with TCP.
  • new_v4_marks_udp_when_not_tcp — locks the is_tcp=falseIPPROTO_UDP branch.
  • new_v6_writes_big_endian_addrs_across_all_four_slots — IPv6 key compared against the same per-slot u32::from_be_bytes formula the old code used.
  • new_icmp_v4_uses_icmp_proto_and_zero_dport — locks IPPROTO_ICMP, dport=0, icmp_id lives in sport.
  • new_icmp_v6_uses_icmpv6_proto_and_zero_dport — same for IPv6 / IPPROTO_ICMPV6.

The test module is inside the existing cfg(target_os = "linux") / feature = "ebpf" gating of the file, so it runs on the Linux CI runner.

Local checks (macOS host):

  • cargo check — clean.
  • cargo clippy --all-targets -- -D warnings — clean.
  • cargo fmt --check — clean.
  • cargo test --lib — 361 passed.

Closes #306

…reachable!() arms

`ConnKey::new` and `ConnKey::new_icmp` both accepted `IpAddr`, matched
`(src_ip, dst_ip)` and panicked on the mixed-family arm via
`unreachable!("Mixed IPv4/IPv6 addresses not supported")`. In practice
every in-repo caller of `new` already went through the typed
`new_v4(Ipv4Addr, Ipv4Addr, ...)` / `new_v6(Ipv6Addr, Ipv6Addr, ...)`
wrappers, so the mixed arm in `new` was unreachable by construction —
it existed only because the inner constructor lost the family
information by widening to `IpAddr`.

Restructure so the type system carries the guarantee:

- Replace `pub fn new(IpAddr, IpAddr, ...)` with two small private
  helpers `empty_v4` / `empty_v6` (build a zeroed key with the right
  `family` and `proto`) plus `fill_v4` / `fill_v6` (write the address
  slots in the family-specific endianness). `new_v4` / `new_v6` call
  these directly — there is no longer a shared `IpAddr`-typed seam.
- Replace `pub fn new_icmp(IpAddr, IpAddr, u16)` with concrete
  `new_icmp_v4(Ipv4Addr, Ipv4Addr, u16)` and
  `new_icmp_v6(Ipv6Addr, Ipv6Addr, u16)`, built on the same helpers.
- Pull the family numbers and `IPPROTO_*` byte values out as named
  constants (`AF_INET` = 2, `AF_INET6` = 10, `IPPROTO_TCP` = 6,
  `IPPROTO_UDP` = 17, `IPPROTO_ICMP` = 1, `IPPROTO_ICMPV6` = 58) so
  the constructors read at the level the kernel headers use.

`tracker_libbpf::lookup_icmp` now dispatches on `(src_ip, dst_ip)` at
the entry point (matching the shape `lookup` already used for the
TCP/UDP path) and forwards to private `lookup_icmp_v4` /
`lookup_icmp_v6` helpers, which pass concrete address types into the
new `new_icmp_v4` / `new_icmp_v6` constructors. The mixed-family case
is now handled exactly like the TCP/UDP path: a `log::warn!` plus
`None` return, no `unreachable!()` panic.

Net effect: both `unreachable!()` arms are gone, the wire-format byte
layout of `ConnKey` is unchanged, and the public surface
(`new_v4` / `new_v6` / new `new_icmp_v4` / new `new_icmp_v6`) is now
typesafe — passing mismatched address families is a compile error
rather than a runtime panic.

Local checks:

- `cargo check` on the macOS host — clean.
- `cargo clippy --all-targets -- -D warnings` — clean.
- `cargo fmt --check` — clean.
- `cargo test --lib` — 361 passed (the Linux-only `#[cfg(test)] mod`
  is exercised on the CI Linux runner; five new cases lock the byte
  layout of `new_v4` / `new_v6` / `new_icmp_v4` / `new_icmp_v6`
  against the previous behaviour).

Closes domcyrus#306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ebpf(maps_libbpf): ConnKey::new keeps two unreachable!() arms that type-safe constructors would eliminate

1 participant