Migrate to net/netip - #1181
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates package-wide IP and prefix representations from stdlib net types to net/netip (netip.Addr, netip.Prefix). Updates parsing, equality, netlink encode/decode, and public APIs across addr, route, link, tc, conntrack, xfrm, socket, ipset, filter, neigh, SEG6, FOU, GTP, rule, and numerous tests. Changes
Sequence Diagram(s)(Skipped — changes are broad type migrations and do not introduce a new multi-component sequential flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
link_test.go (1)
900-918:⚠️ Potential issue | 🟠 MajorSkip PMtuDisc comparison for flow-based tunnels in compareGretap and compareGretun.
The failing tests expose a logic mismatch:
addGretunAttrs()andaddGretapAttrs()skip sending PMtuDisc to the kernel whenFlowBased=true(only sendIFLA_GRE_COLLECT_METADATA), butcompareGretap()andcompareGretun()unconditionally compare PMtuDisc regardless of flow-based mode. The kernel returns a non-zero PMtuDisc default for flow-based tunnels, causing the comparison to fail.Add conditional checks in
compareGretap()(line 503) andcompareGretun()(line 569) to skip PMtuDisc comparison whenFlowBased=true, mirroring the attribute handling in the add functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@link_test.go` around lines 900 - 918, compareGretap() and compareGretun() currently always compare PMtuDisc but addGretapAttrs()/addGretunAttrs() do not send PMtuDisc when FlowBased is true, causing false failures; update compareGretap and compareGretun to skip the PMtuDisc field comparison if the source link has FlowBased == true (i.e., only perform the PMtuDisc equality check when FlowBased is false), mirroring the attribute-send behavior in addGretapAttrs/addGretunAttrs and preventing mismatches against kernel defaults.xfrm_state_linux_test.go (1)
362-374:⚠️ Potential issue | 🔴 CriticalFix XFRMA_SRCADDR encoding to match kernel expectations.
The XFRM state Get/Delete operations fail because XFRMA_SRCADDR is encoded incorrectly. The kernel's netlink validation expects
xfrm_address_t(16 bytes), but line 469 in xfrm_state_linux.go sends only 4 bytes for IPv4 addresses viastate.Src.AsSlice().In contrast, the Add path uses
FromIP(), which properly pads IPv4 addresses to 16 bytes. Use the same approach inxfrmStateGetOrDeleteby creating a properly sized address buffer before passing it toNewRtAttr.Code location
if state.Src.IsValid() { out := nl.NewRtAttr(nl.XFRMA_SRCADDR, state.Src.AsSlice()) req.AddData(out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xfrm_state_linux_test.go` around lines 362 - 374, The XFRMA_SRCADDR attribute is being encoded with state.Src.AsSlice() (which yields 4 bytes for IPv4) inside xfrmStateGetOrDelete, but the kernel expects a 16-byte xfrm_address_t; change the encoding to produce a 16-byte address like the Add path does (use the same FromIP()/16-byte padding approach) before calling nl.NewRtAttr(nl.XFRMA_SRCADDR, ...), i.e. construct a 16-byte buffer from state.Src (pad IPv4 to 16 bytes or use the existing FromIP helper) and pass that buffer to NewRtAttr instead of state.Src.AsSlice().nexthop_test.go (1)
55-60: 🛠️ Refactor suggestion | 🟠 MajorAdd a regression case for the actual
::ffff:gateway bug.These fixtures still only cover native IPv6 and native IPv4 gateways. The issue this PR is supposed to fix is an IPv4-mapped IPv6 gateway being misclassified, so this test will still pass even if
deriveFamilyFromNexthopregresses on::ffff:100.95.128.2.Also applies to: 123-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nexthop_test.go` around lines 55 - 60, Add a regression test for IPv4-mapped IPv6 addresses: create a Nexthop fixture whose Gateway is the IPv4-mapped IPv6 address "::ffff:100.95.128.2" (use netip.MustParseAddr) and assert that deriveFamilyFromNexthop (or the test's family-derivation expectation) classifies it as the IPv4 family, similar to the existing native IPv4/IPv6 cases around the Nexthop fixture at lines showing ID/OIF/Gateway/Protocol; this ensures the ::ffff:... mapping bug is covered.filter_test.go (2)
2141-2158:⚠️ Potential issue | 🟡 MinorApply .Masked() to canonicalize the IPv6 Flower prefix.
netip.MustParsePrefix("ffff::fff1/64")preserves host bits in the address, so it won't compare equal to the canonicalized/64prefix returned by the kernel. This causes the test assertion at line 2189-2190 to fail.Suggested fix
- Dest: netip.MustParsePrefix("ffff::fff1/64"), + Dest: netip.MustParsePrefix("ffff::fff1/64").Masked(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filter_test.go` around lines 2141 - 2158, The Flower test builds a non-canonical IPv6 prefix causing mismatch with kernel-returned canonical prefixes; update the Flower construction to canonicalize the Dest prefix by calling .Masked() on netip.MustParsePrefix("ffff::fff1/64") (i.e., set Dest to netip.MustParsePrefix(...).Masked()) so the prefix compares equal to the kernel's /64 canonical form; locate this in the test where Flower is instantiated (FilterAttrs/ Dest field) and apply the same masking if any other IPv6 prefixes (e.g., peditAction.SrcIP if used as a prefix) are compared to kernel results.
1769-1787:⚠️ Potential issue | 🟡 MinorNormalize the Flower prefixes before asserting on them.
These fixtures keep host bits (
1.0.0.1/24,2.0.0.1/24, etc.), but the kernel round-trip returns masked prefixes. That makes the equality checks at lines 1832-1843 fail even when the kernel state is correct, sincenetip.Prefixequality is strict and compares host bits.Suggested fix
- Dest: netip.MustParsePrefix("1.0.0.1/24"), - Src: netip.MustParsePrefix("2.0.0.1/24"), + Dest: netip.MustParsePrefix("1.0.0.1/24").Masked(), + Src: netip.MustParsePrefix("2.0.0.1/24").Masked(), EthType: unix.ETH_P_IP, - EncDest: netip.MustParsePrefix("3.0.0.1/24"), - EncSrc: netip.MustParsePrefix("4.0.0.1/24"), + EncDest: netip.MustParsePrefix("3.0.0.1/24").Masked(), + EncSrc: netip.MustParsePrefix("4.0.0.1/24").Masked(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filter_test.go` around lines 1769 - 1787, The test constructs a Flower filter with prefixes containing host bits (Dest, Src, EncDest, EncSrc) but compares them to kernel-returned prefixes that have host bits masked; normalize these prefixes before asserting by applying the netip prefix mask operation (i.e., mask the host bits of the expected prefixes) so comparisons use canonical/ masked netip.Prefix values; update the assertions around Flower (fields Dest, Src, EncDest, EncSrc) to compare against the masked versions (or mask the kernel-returned prefixes) so equality is not affected by host-bit differences.route_linux.go (2)
698-718:⚠️ Potential issue | 🟠 MajorReject non-IPv6 addresses in
IP6tnlEncap.Encode.Lines 708-717 hardcode a 16-byte payload, but
netip.Addr.AsSlice()returns 4 bytes for plain IPv4 addresses. If an IPv4 address is passed, the encap attribute will have a declared length of 20 bytes with only 8 bytes of actual payload, creating a malformed packet.Proposed fix
func (e *IP6tnlEncap) Encode() ([]byte, error) { + if e.Dst.BitLen() != net.IPv6len*8 || e.Src.BitLen() != net.IPv6len*8 { + return nil, fmt.Errorf("ip6tnl encap requires IPv6 src and dst") + } final := []byte{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@route_linux.go` around lines 698 - 718, IP6tnlEncap.Encode currently assumes e.Dst and e.Src are IPv6 and writes a 16-byte payload header, but netip.Addr.AsSlice() can return 4 bytes for IPv4 causing malformed attributes; update IP6tnlEncap.Encode to first validate that both e.Dst.Is6() and e.Src.Is6() and return an error if not, and then derive the attribute lengths from the actual address slice lengths when writing resDst and resSrc (i.e. compute size as 2+2+len(addrSlice) before native.PutUint16) so the nl.LWTUNNEL_IP6_SRC and nl.LWTUNNEL_IP6_DST attributes are correct.
781-791:⚠️ Potential issue | 🔴 CriticalFix
Via.Encode()to write address bytes, notnetip.Addrstruct.Line 787 uses
binary.Write(buf, native, v.Addr)on anetip.Addr, which will fail at runtime becausenetip.Addrhas unexported fields thatbinary.Writecannot serialize via reflection. However,Via.Decode(lines 794–809) expects only 4 bytes for IPv4 or 16 bytes for IPv6 after the family field. The current implementation breaks all routes usingVia.Use
v.Addr.AsSlice()to write the wire-format address bytes instead:Proposed fix
func (v *Via) Encode() ([]byte, error) { buf := &bytes.Buffer{} err := binary.Write(buf, native, uint16(v.AddrFamily)) if err != nil { return nil, err } - err = binary.Write(buf, native, v.Addr) - if err != nil { - return nil, err + switch v.AddrFamily { + case nl.FAMILY_V4: + if v.Addr.BitLen() != net.IPv4len*8 { + return nil, fmt.Errorf("encoding failed: address family %d requires an IPv4 address", v.AddrFamily) + } + case nl.FAMILY_V6: + if v.Addr.BitLen() != net.IPv6len*8 { + return nil, fmt.Errorf("encoding failed: address family %d requires an IPv6 address", v.AddrFamily) + } + default: + return nil, fmt.Errorf("encoding failed: address family %d unknown", v.AddrFamily) + } + if _, err := buf.Write(v.Addr.AsSlice()); err != nil { + return nil, err } return buf.Bytes(), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@route_linux.go` around lines 781 - 791, Via.Encode() currently calls binary.Write(buf, native, v.Addr) which attempts to serialize a netip.Addr struct (with unexported fields) and will fail; instead write the raw wire-format address bytes so Decode (Via.Decode) which expects 4 bytes for IPv4 or 16 bytes for IPv6 can read them. Replace the binary.Write of v.Addr with writing v.Addr.AsSlice() (or the slice returned by v.Addr.AsSlice() for the appropriate family) into buf, preserving the initial uint16(v.AddrFamily) write and returning buf.Bytes().
🧹 Nitpick comments (6)
socket_linux.go (1)
187-199: Consider explicit nil-IP handling for clarity.The
okreturn fromnetip.AddrFromSlice()is ignored. Ifl.IPorr.IPisnil,AddrFromSlicereturns an invalid zero-valuenetip.Addr, which will fail bothIs4()andIs6()checks, ultimately returningErrNotImplemented.While the current behavior is safe, explicit validation would make the failure mode clearer:
♻️ Optional: explicit nil handling
- localIP, _ = netip.AddrFromSlice(l.IP) + localIP, ok := netip.AddrFromSlice(l.IP) + if !ok { + return nil, ErrNotImplemented + } localPort = uint16(l.Port) - remoteIP, _ = netip.AddrFromSlice(r.IP) + remoteIP, ok = netip.AddrFromSlice(r.IP) + if !ok { + return nil, ErrNotImplemented + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@socket_linux.go` around lines 187 - 199, The code ignores the boolean result from netip.AddrFromSlice when converting l.IP and r.IP (used to set localIP and remoteIP), which silently produces invalid addrs if the underlying byte slices are nil; update the TCP and UDP address handling to explicitly check the ok return from netip.AddrFromSlice for both l.IP and r.IP and return ErrNotImplemented (or a descriptive error) when ok is false so failures are explicit; locate the conversion sites using netip.AddrFromSlice and variables localIP, remoteIP, localPort, remotePort and ensure both conversions validate ok before proceeding (same change in the TCP and UDP switch cases).gtp_test.go (1)
21-37: These tests should assert the migrated address semantics, not just non-nil lookups.Right now they only swap constructors and confirm lookup succeeds. That will not catch a broken
AsSlice()/AddrFromSlice()round-trip. Please assert that the returnedPDPcarries the expectedPeerAddress/MSAddress, and add at least one::ffff:x.y.z.wcase if this path is supposed to preserve the mapped-address behavior the PR is targeting.Also applies to: 73-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gtp_test.go` around lines 21 - 37, Update the tests that call GTPPDPAdd, GTPPDPList and GTPPDPByMSAddress to assert round-tripped address semantics instead of just non-nil lookups: after adding a PDP (constructed with PeerAddress and MSAddress) verify the returned PDP's PeerAddress and MSAddress exactly equal the originals (use the PDP struct fields), and repeat the assertions for the other test block as well; additionally add a test case that uses an IPv4-mapped IPv6 address (::ffff:x.y.z.w) to ensure AsSlice()/AddrFromSlice() preserve mapped-address behavior when looked up via GTPPDPByMSAddress.fou_linux.go (1)
191-194: Silently discarding parse errors is acceptable but consumers must checkIsValid().
netip.AddrFromSlicereturns a boolean indicating success, which is discarded here. On failure (e.g., unexpected byte length),fou.Local/fou.Peerwill be an invalid zeroAddr. This is safe as long as consumers callIsValid()before using these fields.Per the context snippet from
link_linux.go, other address fields likeGretap.Localfollow a similar pattern whereIsValid()checks are performed before use. Consider documenting thatFou.Local/Fou.Peermay be invalid if the kernel sends unexpected data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fou_linux.go` around lines 191 - 194, The code currently discards the boolean result from netip.AddrFromSlice when setting fou.Local and fou.Peer for FOU_ATTR_LOCAL_V4/6 and FOU_ATTR_PEER_V4/6, which can leave those fields as an invalid zero Addr; update the codebase by adding a concise comment or documentation next to the fou struct fields (or adjacent to the switch handling those attributes) stating that fou.Local and fou.Peer may be invalid if the kernel provides unexpected data and callers must call IsValid() on those fields before use; reference the symbols fou.Local, fou.Peer, netip.AddrFromSlice and IsValid() so reviewers can locate and rely on the documented behavior.nl/xfrm_linux.go (1)
130-142: Consider checkingAddrFromSlicesuccess status.The second return value from
netip.AddrFromSliceis discarded. If the slice length is unexpected, this silently produces an invalid address rather than signaling an error. While this may be acceptable for internal usage where slice lengths are controlled, explicit handling would be more defensive.addr, ok := netip.AddrFromSlice(x[0:4]) if !ok { return netip.Addr{} // or handle error } return addr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nl/xfrm_linux.go` around lines 130 - 142, In XfrmAddress.ToIP the call to netip.AddrFromSlice discards the ok boolean; change the ipv4 branch in ToIP to check the second return (ok) from netip.AddrFromSlice and handle failure explicitly (e.g., return netip.Addr{} or another sentinel) instead of silently using an invalid addr, so that AddrFromSlice(x[0:4]) is validated before returning.link_linux.go (1)
3052-3058: Consider handlingAddrFromSlicefailure for robustness.The
okreturn value fromnetip.AddrFromSliceis discarded. While netlink typically provides valid address data, explicitly checking could help catch unexpected data:if addr, ok := netip.AddrFromSlice(datum.Value[0:4]); ok { vxlan.SrcAddr = addr }This pattern is used correctly in
parseBondArpIpTargets(line 3173) but inconsistently applied elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@link_linux.go` around lines 3052 - 3058, The code currently ignores the boolean return from netip.AddrFromSlice when parsing VXLAN addresses (in the switch cases handling nl.IFLA_VXLAN_LOCAL, nl.IFLA_VXLAN_LOCAL6, nl.IFLA_VXLAN_GROUP, nl.IFLA_VXLAN_GROUP6), so update those cases to check the ok value and only assign to vxlan.SrcAddr or vxlan.Group when AddrFromSlice returns ok (same pattern used in parseBondArpIpTargets). Ensure you slice the correct bytes (0:4 for IPv4, 0:16 for IPv6) and avoid discarding errors by guarding the assignment with the ok check.filter_linux.go (1)
1043-1045: Consider checkingAddrFromSlicesuccess for consistency.The
okreturn value is discarded here, unlike the thorough error handling inFlower.decode. For consistency:case nl.TCA_TUNNEL_KEY_ENC_IPV6_SRC, nl.TCA_TUNNEL_KEY_ENC_IPV4_SRC: - action.(*TunnelKeyAction).SrcAddr, _ = netip.AddrFromSlice(adatum.Value) + if addr, ok := netip.AddrFromSlice(adatum.Value); ok { + action.(*TunnelKeyAction).SrcAddr = addr + } case nl.TCA_TUNNEL_KEY_ENC_IPV6_DST, nl.TCA_TUNNEL_KEY_ENC_IPV4_DST: - action.(*TunnelKeyAction).DstAddr, _ = netip.AddrFromSlice(adatum.Value) + if addr, ok := netip.AddrFromSlice(adatum.Value); ok { + action.(*TunnelKeyAction).DstAddr = addr + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filter_linux.go` around lines 1043 - 1045, The code assigns netip.AddrFromSlice results to TunnelKeyAction.SrcAddr/DstAddr while discarding the ok flag; update the assignments in the block that touches action.(*TunnelKeyAction).SrcAddr and .DstAddr to capture the second return (ok), verify it, and handle failures consistently with Flower.decode (e.g., return or propagate an error or log and skip) so malformed adatum.Value is not silently accepted; reference the AddrFromSlice call sites and the TunnelKeyAction type when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gtp_linux.go`:
- Line 186: The netlink attributes GENL_GTP_ATTR_MS_ADDRESS and
GENL_GTP_ATTR_PEER_ADDRESS must carry 4-byte IPv4 addresses only; update the
serialization in gtp_linux.go where you call
req.AddData(nl.NewRtAttr(nl.GENL_GTP_ATTR_MS_ADDRESS, addr.AsSlice())) (and the
similar occurrences around lines 207–208) to either normalize IPv4-mapped
addresses by using addr.Unmap().AsSlice() before passing to NewRtAttr, or
explicitly validate addr is IPv4 and return an error if not; ensure you apply
the same change for both MS_ADDRESS and PEER_ADDRESS usages so only 4-byte
payloads are sent.
In `@ipset_linux_test.go`:
- Around line 651-653: The bitmap:ip test condition is inverted causing exact
matches to fail; in ipset_linux_test.go locate the branch checking tC.typename
== "bitmap:ip" and change the equality checks on result.IPFrom/result.IPTo
against tC.options.IPFrom/tC.options.IPTo to use != (not equal) so the condition
becomes true only when the result addresses are invalid or do not match the
expected range; update the comparisons for both IPFrom and IPTo in that if
expression.
In `@neigh_linux.go`:
- Around line 339-346: The NDA_LLADDR branch currently parses the address and
assigns it to neigh.IP, which overwrites the real NDA_DST; instead, when
LinkByIndex(neigh.LinkIndex) returns a link with link.Attrs().EncapType ==
"tunnel6", set the parsed netip.Addr into neigh.LLIPAddr (not neigh.IP) so the
tunnel6 link-layer IP is preserved separately; update the NDA_LLADDR handling to
assign addr to neigh.LLIPAddr and leave neigh.IP untouched to allow correct
round-tripping.
In `@neigh_test.go`:
- Around line 127-133: The test's arpTable (arpEntry slice constructed using
netip.MustParseAddr and parseMAC) only contains native IPv4 addresses and
doesn't exercise the IPv4-mapped IPv6 neighbor path; add an entry using an
IPv4-mapped IPv6 address (e.g. netip.MustParseAddr("::ffff:10.99.0.6")) with a
corresponding MAC via parseMAC to the arpTable so the neigh handling regression
for To4()-style misclassification is covered (update any expectations/assertions
that count or lookup these entries accordingly).
In `@nl/xfrm_linux.go`:
- Around line 145-158: In XfrmAddress.ToIPNet, the code currently ignores the
supplied prefixlen for non-empty addresses and always returns host masks; update
ToIPNet (function: ToIPNet on type XfrmAddress) to pass the provided prefixlen
into netip.PrefixFrom by converting prefixlen to int (e.g., netip.PrefixFrom(ip,
int(prefixlen))) instead of hardcoding 32/128, preserving the original prefix
length returned by ToIP().
In `@route_linux.go`:
- Around line 1295-1301: The branch handling RT_FILTER_DST is clobbering an
explicitly provided filter.Dst because the validity check is inverted; change
the logic so you only call genZeroIPNet(family) when filter.Dst is not set
(i.e., when filter.Dst.IsValid() is false) instead of when it is true. Locate
the RT_FILTER_DST case that references filter.MPLSDst, route.MPLSDst,
filter.Dst, genZeroIPNet and family and flip the condition from "if
filter.Dst.IsValid()" to "if !filter.Dst.IsValid()" (or equivalent) so callers'
explicit filter.Dst values are preserved and defaulting only occurs when
filter.Dst is the zero value.
In `@route_test.go`:
- Around line 503-504: The assertion in the test after calling RouteAppend is
inverted: it checks routes[1].Src == ip which fails when append succeeded;
update the test so it asserts routes[1].Src == ip (or flip the boolean) and keep
the length check (len(routes) == 2). Locate the test that calls RouteAppend and
change the condition around routes, routes[1].Src and ip to expect equality
(routes[1].Src == ip) for a successful append.
- Around line 446-447: The test assertion is inverted: after calling
RouteReplace, routes should contain exactly one entry and that entry's Src must
equal ip; change the condition in the failing check to detect a mismatch (e.g.
use routes[0].Src != ip) so the test fails when replacement did not happen.
Update the check that currently reads with routes and routes[0].Src (after
RouteReplace) to assert routes length == 1 and routes[0].Src == ip, failing when
either len(routes) != 1 or routes[0].Src != ip.
- Around line 543-551: The helper expectRouteUpdate currently only compares
update.Route.Dst.Addr() to the provided dst, losing prefix length information;
update the comparison to include the full prefix (e.g., compare update.Route.Dst
to dst as a netip.Prefix or compare both Addr() and Bits()/MaskLength) so /16 vs
/24 are distinguished, and adjust callers so they pass the prefix (dst) itself
rather than dst.Addr(); locate the check in expectRouteUpdate and replace the
Addr-only comparison with a full-prefix equality check.
In `@rule_linux.go`:
- Around line 237-248: The code silently ignores errors from addr.Prefix when
decoding nl.FRA_SRC and nl.FRA_DST; modify the FRA_SRC and FRA_DST cases so you
call addr.Prefix(int(msg.Src_len)) and addr.Prefix(int(msg.Dst_len)) and check
the returned error, returning a parse error (e.g., fmt.Errorf with context) if
Prefix returns an error instead of discarding it; update the assignments to
rule.Src and rule.Dst only after Prefix succeeds to ensure invalid/out-of-range
prefix lengths are propagated as parse failures.
- Around line 66-83: The code encodes rule.Dst.Addr().AsSlice() and
rule.Src.Addr().AsSlice() without applying netip.Prefix.Masked(), causing host
bits to be preserved and mismatching kernel-canonicalized prefixes; update the
encoding in the rule handling (the block that sets msg.Dst_len/msg.Src_len,
msg.Family, dstFamily and appends to rtAttrs via nl.NewRtAttr with unix.RTA_DST
and unix.RTA_SRC) to call Masked() on rule.Dst and rule.Src first and use the
masked prefix for Bits() and Addr().AsSlice() so the netlink payload uses the
canonical network address.
- Around line 377-386: The comparison in ruleIPNetEqual should compare
canonicalized (masked) prefixes, not the raw netip.Prefix values; update
ruleIPNetEqual to canonicalize both inputs (e.g., call the
Prefix.Masked()/Masked form or otherwise zero host bits using the prefix's Addr
and Bits) and then compare those masked prefixes for equality (keep the existing
default/invalid handling and replace the final raw equality check with a
comparison of the masked/canonicalized prefixes).
In `@rule_test.go`:
- Around line 676-679: The comparisons for a.Src and a.Dst should use the
existing ruleIPNetEqual helper instead of comparing Prefix.String() to handle
canonicalization (e.g., 172.16.0.1/16 vs 172.16.0.0/16); replace the Src and Dst
boolean expressions that currently check IsValid()/String() equality with calls
to ruleIPNetEqual(a.Src, b.Src) and ruleIPNetEqual(a.Dst, b.Dst) so prefix
equivalence matches kernel-normalized rules (this will fix TestRuleAddDel).
In `@xfrm_policy_linux_test.go`:
- Around line 221-225: The equality check in the test compares netip.Prefix
directly (the return expression that checks a.Src == b.Src && a.Dst == b.Dst),
which fails when one side is an unset prefix and the other is a concrete /0; add
a helper like samePrefix(a,b netip.Prefix) that treats an invalid (unset) prefix
and a valid unspecified /0 as equal, then replace the direct checks in the
return statement with samePrefix(a.Src, b.Src) and samePrefix(a.Dst, b.Dst) so
policy round-trips are considered equivalent.
In `@xfrm_policy_linux.go`:
- Around line 104-112: The selector family (sel.Family) is only derived from
policy.Dst causing src-only IPv6 policies to be serialized with Family=IPv4;
update the logic in xfrm_policy_linux.go so that after defaulting sel.Family =
uint16(nl.FAMILY_V4) you set sel.Family =
uint16(nl.GetIPFamily(policy.Dst.Addr())) when policy.Dst.IsValid(), otherwise
if policy.Src.IsValid() set sel.Family =
uint16(nl.GetIPFamily(policy.Src.Addr())); keep existing assignments to
sel.Saddr/PrefixlenS and sel.Daddr/PrefixlenD unchanged.
In `@xfrm_state_linux.go`:
- Around line 468-471: The XFRMA_SRCADDR attribute is currently sent using
state.Src.AsSlice(), which yields variable-length (4 or 16 bytes); replace this
with a 16-byte xfrm_address_t by creating an nl.XfrmAddress, calling its
FromIP(state.Src) to serialize/zero-pad IPv4 to 16 bytes, and pass srcAddr[:]
into nl.NewRtAttr for nl.XFRMA_SRCADDR before calling req.AddData; update the
block that references state.Src, nl.XFRMA_SRCADDR, nl.XfrmAddress, FromIP,
nl.NewRtAttr and req.AddData accordingly.
---
Outside diff comments:
In `@filter_test.go`:
- Around line 2141-2158: The Flower test builds a non-canonical IPv6 prefix
causing mismatch with kernel-returned canonical prefixes; update the Flower
construction to canonicalize the Dest prefix by calling .Masked() on
netip.MustParsePrefix("ffff::fff1/64") (i.e., set Dest to
netip.MustParsePrefix(...).Masked()) so the prefix compares equal to the
kernel's /64 canonical form; locate this in the test where Flower is
instantiated (FilterAttrs/ Dest field) and apply the same masking if any other
IPv6 prefixes (e.g., peditAction.SrcIP if used as a prefix) are compared to
kernel results.
- Around line 1769-1787: The test constructs a Flower filter with prefixes
containing host bits (Dest, Src, EncDest, EncSrc) but compares them to
kernel-returned prefixes that have host bits masked; normalize these prefixes
before asserting by applying the netip prefix mask operation (i.e., mask the
host bits of the expected prefixes) so comparisons use canonical/ masked
netip.Prefix values; update the assertions around Flower (fields Dest, Src,
EncDest, EncSrc) to compare against the masked versions (or mask the
kernel-returned prefixes) so equality is not affected by host-bit differences.
In `@link_test.go`:
- Around line 900-918: compareGretap() and compareGretun() currently always
compare PMtuDisc but addGretapAttrs()/addGretunAttrs() do not send PMtuDisc when
FlowBased is true, causing false failures; update compareGretap and
compareGretun to skip the PMtuDisc field comparison if the source link has
FlowBased == true (i.e., only perform the PMtuDisc equality check when FlowBased
is false), mirroring the attribute-send behavior in
addGretapAttrs/addGretunAttrs and preventing mismatches against kernel defaults.
In `@nexthop_test.go`:
- Around line 55-60: Add a regression test for IPv4-mapped IPv6 addresses:
create a Nexthop fixture whose Gateway is the IPv4-mapped IPv6 address
"::ffff:100.95.128.2" (use netip.MustParseAddr) and assert that
deriveFamilyFromNexthop (or the test's family-derivation expectation) classifies
it as the IPv4 family, similar to the existing native IPv4/IPv6 cases around the
Nexthop fixture at lines showing ID/OIF/Gateway/Protocol; this ensures the
::ffff:... mapping bug is covered.
In `@route_linux.go`:
- Around line 698-718: IP6tnlEncap.Encode currently assumes e.Dst and e.Src are
IPv6 and writes a 16-byte payload header, but netip.Addr.AsSlice() can return 4
bytes for IPv4 causing malformed attributes; update IP6tnlEncap.Encode to first
validate that both e.Dst.Is6() and e.Src.Is6() and return an error if not, and
then derive the attribute lengths from the actual address slice lengths when
writing resDst and resSrc (i.e. compute size as 2+2+len(addrSlice) before
native.PutUint16) so the nl.LWTUNNEL_IP6_SRC and nl.LWTUNNEL_IP6_DST attributes
are correct.
- Around line 781-791: Via.Encode() currently calls binary.Write(buf, native,
v.Addr) which attempts to serialize a netip.Addr struct (with unexported fields)
and will fail; instead write the raw wire-format address bytes so Decode
(Via.Decode) which expects 4 bytes for IPv4 or 16 bytes for IPv6 can read them.
Replace the binary.Write of v.Addr with writing v.Addr.AsSlice() (or the slice
returned by v.Addr.AsSlice() for the appropriate family) into buf, preserving
the initial uint16(v.AddrFamily) write and returning buf.Bytes().
In `@xfrm_state_linux_test.go`:
- Around line 362-374: The XFRMA_SRCADDR attribute is being encoded with
state.Src.AsSlice() (which yields 4 bytes for IPv4) inside xfrmStateGetOrDelete,
but the kernel expects a 16-byte xfrm_address_t; change the encoding to produce
a 16-byte address like the Add path does (use the same FromIP()/16-byte padding
approach) before calling nl.NewRtAttr(nl.XFRMA_SRCADDR, ...), i.e. construct a
16-byte buffer from state.Src (pad IPv4 to 16 bytes or use the existing FromIP
helper) and pass that buffer to NewRtAttr instead of state.Src.AsSlice().
---
Nitpick comments:
In `@filter_linux.go`:
- Around line 1043-1045: The code assigns netip.AddrFromSlice results to
TunnelKeyAction.SrcAddr/DstAddr while discarding the ok flag; update the
assignments in the block that touches action.(*TunnelKeyAction).SrcAddr and
.DstAddr to capture the second return (ok), verify it, and handle failures
consistently with Flower.decode (e.g., return or propagate an error or log and
skip) so malformed adatum.Value is not silently accepted; reference the
AddrFromSlice call sites and the TunnelKeyAction type when making the change.
In `@fou_linux.go`:
- Around line 191-194: The code currently discards the boolean result from
netip.AddrFromSlice when setting fou.Local and fou.Peer for FOU_ATTR_LOCAL_V4/6
and FOU_ATTR_PEER_V4/6, which can leave those fields as an invalid zero Addr;
update the codebase by adding a concise comment or documentation next to the fou
struct fields (or adjacent to the switch handling those attributes) stating that
fou.Local and fou.Peer may be invalid if the kernel provides unexpected data and
callers must call IsValid() on those fields before use; reference the symbols
fou.Local, fou.Peer, netip.AddrFromSlice and IsValid() so reviewers can locate
and rely on the documented behavior.
In `@gtp_test.go`:
- Around line 21-37: Update the tests that call GTPPDPAdd, GTPPDPList and
GTPPDPByMSAddress to assert round-tripped address semantics instead of just
non-nil lookups: after adding a PDP (constructed with PeerAddress and MSAddress)
verify the returned PDP's PeerAddress and MSAddress exactly equal the originals
(use the PDP struct fields), and repeat the assertions for the other test block
as well; additionally add a test case that uses an IPv4-mapped IPv6 address
(::ffff:x.y.z.w) to ensure AsSlice()/AddrFromSlice() preserve mapped-address
behavior when looked up via GTPPDPByMSAddress.
In `@link_linux.go`:
- Around line 3052-3058: The code currently ignores the boolean return from
netip.AddrFromSlice when parsing VXLAN addresses (in the switch cases handling
nl.IFLA_VXLAN_LOCAL, nl.IFLA_VXLAN_LOCAL6, nl.IFLA_VXLAN_GROUP,
nl.IFLA_VXLAN_GROUP6), so update those cases to check the ok value and only
assign to vxlan.SrcAddr or vxlan.Group when AddrFromSlice returns ok (same
pattern used in parseBondArpIpTargets). Ensure you slice the correct bytes (0:4
for IPv4, 0:16 for IPv6) and avoid discarding errors by guarding the assignment
with the ok check.
In `@nl/xfrm_linux.go`:
- Around line 130-142: In XfrmAddress.ToIP the call to netip.AddrFromSlice
discards the ok boolean; change the ipv4 branch in ToIP to check the second
return (ok) from netip.AddrFromSlice and handle failure explicitly (e.g., return
netip.Addr{} or another sentinel) instead of silently using an invalid addr, so
that AddrFromSlice(x[0:4]) is validated before returning.
In `@socket_linux.go`:
- Around line 187-199: The code ignores the boolean result from
netip.AddrFromSlice when converting l.IP and r.IP (used to set localIP and
remoteIP), which silently produces invalid addrs if the underlying byte slices
are nil; update the TCP and UDP address handling to explicitly check the ok
return from netip.AddrFromSlice for both l.IP and r.IP and return
ErrNotImplemented (or a descriptive error) when ok is false so failures are
explicit; locate the conversion sites using netip.AddrFromSlice and variables
localIP, remoteIP, localPort, remotePort and ensure both conversions validate ok
before proceeding (same change in the TCP and UDP switch cases).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13a5cd89-78c4-4925-ad0b-39e0f4d171bc
📒 Files selected for processing (48)
addr.goaddr_linux.goaddr_test.gobridge_linux_test.gocmd/ipset-test/main.goconntrack_linux.goconntrack_test.gofilter.gofilter_linux.gofilter_test.gofou.gofou_linux.gofou_test.gogtp_linux.gogtp_test.gohandle_test.gohandle_unspecified.goipset_linux.goipset_linux_test.golink.golink_linux.golink_test.goneigh.goneigh_linux.goneigh_test.gonetlink.gonetlink_unspecified.gonexthop.gonexthop_linux.gonexthop_test.gonl/nl_linux.gonl/seg6_linux.gonl/tc_linux.gonl/tc_linux_test.gonl/xfrm_linux.goroute.goroute_linux.goroute_test.gorule.gorule_linux.gorule_test.gosocket.gosocket_linux.gosocket_test.goxfrm_policy_linux.goxfrm_policy_linux_test.goxfrm_state_linux.goxfrm_state_linux_test.go
| } else if tC.typename == "bitmap:ip" { | ||
| if result.IPFrom == nil || result.IPTo == nil || result.IPFrom.Equal(tC.options.IPFrom) || result.IPTo.Equal(tC.options.IPTo) { | ||
| if !result.IPFrom.IsValid() || !result.IPTo.IsValid() || result.IPFrom == tC.options.IPFrom || result.IPTo == tC.options.IPTo { | ||
| t.Fatalf("expected ip range %v-%v, got %v-%v", tC.options.IPFrom, tC.options.IPTo, result.IPFrom, result.IPTo) |
There was a problem hiding this comment.
Flip the bitmap:ip equality check.
This condition currently fails when result.IPFrom/result.IPTo exactly match the expected range, so the first bitmap:ip case added here will report a false failure.
Suggested fix
- if !result.IPFrom.IsValid() || !result.IPTo.IsValid() || result.IPFrom == tC.options.IPFrom || result.IPTo == tC.options.IPTo {
+ if !result.IPFrom.IsValid() || !result.IPTo.IsValid() || result.IPFrom != tC.options.IPFrom || result.IPTo != tC.options.IPTo {
t.Fatalf("expected ip range %v-%v, got %v-%v", tC.options.IPFrom, tC.options.IPTo, result.IPFrom, result.IPTo)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if tC.typename == "bitmap:ip" { | |
| if result.IPFrom == nil || result.IPTo == nil || result.IPFrom.Equal(tC.options.IPFrom) || result.IPTo.Equal(tC.options.IPTo) { | |
| if !result.IPFrom.IsValid() || !result.IPTo.IsValid() || result.IPFrom == tC.options.IPFrom || result.IPTo == tC.options.IPTo { | |
| t.Fatalf("expected ip range %v-%v, got %v-%v", tC.options.IPFrom, tC.options.IPTo, result.IPFrom, result.IPTo) | |
| } else if tC.typename == "bitmap:ip" { | |
| if !result.IPFrom.IsValid() || !result.IPTo.IsValid() || result.IPFrom != tC.options.IPFrom || result.IPTo != tC.options.IPTo { | |
| t.Fatalf("expected ip range %v-%v, got %v-%v", tC.options.IPFrom, tC.options.IPTo, result.IPFrom, result.IPTo) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ipset_linux_test.go` around lines 651 - 653, The bitmap:ip test condition is
inverted causing exact matches to fail; in ipset_linux_test.go locate the branch
checking tC.typename == "bitmap:ip" and change the equality checks on
result.IPFrom/result.IPTo against tC.options.IPFrom/tC.options.IPTo to use !=
(not equal) so the condition becomes true only when the result addresses are
invalid or do not match the expected range; update the comparisons for both
IPFrom and IPTo in that if expression.
There was a problem hiding this comment.
I believe this "issue" was already present prior this change (or there is a difference of how were done comparisons between net.IP Equal() and now with netip.Addr)
There was a problem hiding this comment.
@fdomain, you're right — the original code used .Equal() (not !.Equal()), so the inverted logic was already there before this PR, and the migration faithfully preserved the same semantics (nil → !IsValid(), .Equal() → ==). The translation is correct.
Worth noting as well: this else if tC.typename == "bitmap:ip" branch is effectively dead code in TestIpsetBitmapCreateListWithTestCases — none of the test cases in that table use "bitmap:ip" as the typename (they all use "bitmap:port"), so this condition never actually executes. That's likely why the pre-existing inversion went unnoticed.
Would you like me to open a follow-up issue to either fix the inverted condition or add a bitmap:ip test case so the branch is actually exercised?
| link, err := LinkByIndex(neigh.LinkIndex) | ||
| if err == nil && link.Attrs().EncapType == "tunnel6" { | ||
| neigh.IP = net.IP(attr.Value) | ||
| addr, ok := netip.AddrFromSlice(attr.Value) | ||
| if !ok { | ||
| return nil, fmt.Errorf("NDA_LLADDR: invalid address") | ||
| } | ||
|
|
||
| neigh.IP = addr |
There was a problem hiding this comment.
Populate LLIPAddr from tunnel6 NDA_LLADDR.
This branch is decoding NDA_LLADDR, but it stores the parsed address into neigh.IP. That loses the real NDA_DST value and breaks round-tripping for neighbors that use IP-valued link-layer addresses.
Suggested fix
- neigh.IP = addr
+ neigh.LLIPAddr = addr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| link, err := LinkByIndex(neigh.LinkIndex) | |
| if err == nil && link.Attrs().EncapType == "tunnel6" { | |
| neigh.IP = net.IP(attr.Value) | |
| addr, ok := netip.AddrFromSlice(attr.Value) | |
| if !ok { | |
| return nil, fmt.Errorf("NDA_LLADDR: invalid address") | |
| } | |
| neigh.IP = addr | |
| link, err := LinkByIndex(neigh.LinkIndex) | |
| if err == nil && link.Attrs().EncapType == "tunnel6" { | |
| addr, ok := netip.AddrFromSlice(attr.Value) | |
| if !ok { | |
| return nil, fmt.Errorf("NDA_LLADDR: invalid address") | |
| } | |
| neigh.LLIPAddr = addr |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neigh_linux.go` around lines 339 - 346, The NDA_LLADDR branch currently
parses the address and assigns it to neigh.IP, which overwrites the real
NDA_DST; instead, when LinkByIndex(neigh.LinkIndex) returns a link with
link.Attrs().EncapType == "tunnel6", set the parsed netip.Addr into
neigh.LLIPAddr (not neigh.IP) so the tunnel6 link-layer IP is preserved
separately; update the NDA_LLADDR handling to assign addr to neigh.LLIPAddr and
leave neigh.IP untouched to allow correct round-tripping.
There was a problem hiding this comment.
This logic was existing before this change, not a regression.
There was a problem hiding this comment.
@fdomain, you're right — looking at the diff, the original code was neigh.IP = net.IP(attr.Value) and the new code preserves exactly that behavior with neigh.IP = addr. This PR is only migrating the type, not changing the assignment target. I apologize for the noise.
If the pre-existing assignment target (neigh.IP vs neigh.LLIPAddr) is worth revisiting, that would be a separate follow-up. Would you like me to open an issue to track it?
( ••)
( ••)>⌐■-■
(⌐■-■)
🧠 Learnings used
Learnt from: kyrtapz
Repo: vishvananda/netlink PR: 1159
File: bridge_linux.go:349-366
Timestamp: 2026-02-06T09:13:09.289Z
Learning: VNI filter operations (RTM_NEWTUNNEL, RTM_DELTUNNEL, RTM_GETTUNNEL) in bridge_linux.go do not require NLM_F_CREATE or NLM_F_APPEND flags. They only need NLM_F_ACK for acknowledgment, as confirmed by the iproute2 reference implementation in bridge/vni.c.
Learnt from: mattedallo
Repo: vishvananda/netlink PR: 1164
File: bridge_linux.go:44-48
Timestamp: 2026-02-09T08:32:40.092Z
Learning: In bridge_linux.go, BridgeVlanTunnelShow and BridgeVlanTunnelShowDev correctly use RTEXT_FILTER_BRVLAN (not RTEXT_FILTER_BRVLAN_COMPRESSED) for tunnel info dumps, matching iproute2's bridge vlan tunnelshow default behavior. RTEXT_FILTER_BRVLAN_COMPRESSED is only used with the -compressvlans flag to control VLAN range compression in display output, not tunnel info availability.
| arpTable := []arpEntry{ | ||
| {net.ParseIP("10.99.0.1"), parseMAC("aa:bb:cc:dd:00:01")}, | ||
| {net.ParseIP("10.99.0.2"), parseMAC("aa:bb:cc:dd:00:02")}, | ||
| {net.ParseIP("10.99.0.3"), parseMAC("aa:bb:cc:dd:00:03")}, | ||
| {net.ParseIP("10.99.0.4"), parseMAC("aa:bb:cc:dd:00:04")}, | ||
| {net.ParseIP("10.99.0.5"), parseMAC("aa:bb:cc:dd:00:05")}, | ||
| {netip.MustParseAddr("10.99.0.1"), parseMAC("aa:bb:cc:dd:00:01")}, | ||
| {netip.MustParseAddr("10.99.0.2"), parseMAC("aa:bb:cc:dd:00:02")}, | ||
| {netip.MustParseAddr("10.99.0.3"), parseMAC("aa:bb:cc:dd:00:03")}, | ||
| {netip.MustParseAddr("10.99.0.4"), parseMAC("aa:bb:cc:dd:00:04")}, | ||
| {netip.MustParseAddr("10.99.0.5"), parseMAC("aa:bb:cc:dd:00:05")}, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Cover the IPv4-mapped IPv6 neighbor path too.
One of the stated objectives is fixing To4()-style misclassification in neigh handling, but this table still only exercises native IPv4 entries. Please add a ::ffff: neighbor fixture here so this file actually guards the regression being fixed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neigh_test.go` around lines 127 - 133, The test's arpTable (arpEntry slice
constructed using netip.MustParseAddr and parseMAC) only contains native IPv4
addresses and doesn't exercise the IPv4-mapped IPv6 neighbor path; add an entry
using an IPv4-mapped IPv6 address (e.g. netip.MustParseAddr("::ffff:10.99.0.6"))
with a corresponding MAC via parseMAC to the arpTable so the neigh handling
regression for To4()-style misclassification is covered (update any
expectations/assertions that count or lookup these entries accordingly).
There was a problem hiding this comment.
Not sure this is relevant as well for this change, it's a suggestion to improve the coverage.
There was a problem hiding this comment.
@fdomain, that's a fair point. The migration itself (switching from To4()/AsSlice() in neigh_linux.go) is the actual fix, so the test is purely additive coverage. However, since the PR's stated objectives explicitly call out neighbor handling as one of the To4()-misclassification paths being addressed (per issue #1114), a regression test here would directly validate that specific fix and guard against future regressions.
That said, it's entirely your call — if you'd prefer to keep it out of scope for this PR, that's reasonable. Would you like me to open a follow-up issue to track adding IPv4-mapped IPv6 coverage for the neighbor path instead?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nl/tc_linux.go (1)
1626-1656:⚠️ Potential issue | 🟠 MajorGuard the explicit IPv4 setters against IPv6-family input.
Addr.AsSlice()returns 16 bytes for IPv6 and IPv4-mapped IPv6 addresses. CallingUint32(ip.AsSlice())on such addresses reads the first 4 bytes and silently encodes the wrong value. The oldTo4()path unwrapped mapped addresses and rejected plain IPv6 instead.Suggested fix
func (p *TcPedit) SetIPv4Src(ip netip.Addr) { - u32 := NativeEndian().Uint32(ip.AsSlice()) + ip = ip.Unmap() + if !ip.Is4() { + return + } + ip4 := ip.As4() + u32 := NativeEndian().Uint32(ip4[:]) tKey := TcPeditKey{} tKeyEx := TcPeditKeyEx{} @@ func (p *TcPedit) SetIPv4Dst(ip netip.Addr) { - u32 := NativeEndian().Uint32(ip.AsSlice()) + ip = ip.Unmap() + if !ip.Is4() { + return + } + ip4 := ip.As4() + u32 := NativeEndian().Uint32(ip4[:]) tKey := TcPeditKey{} tKeyEx := TcPeditKeyEx{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nl/tc_linux.go` around lines 1626 - 1656, The IPv4 setter functions SetIPv4Src and SetIPv4Dst must reject or unwrap IPv6 inputs before reading 4 bytes; call ip.Unmap() (or use ip.As4()/ip.Is4()) and verify you have a valid IPv4 address before calling NativeEndian().Uint32 on the byte slice. Update both TcPedit.SetIPv4Src and TcPedit.SetIPv4Dst to obtain the 4-byte IPv4 form (e.g., via v4, ok := ip.As4() or ip = ip.Unmap(); v4, ok := ip.As4()), return or error when ok is false, then use that 4-byte value to populate tKey.Val and proceed as before.route_linux.go (1)
781-788:⚠️ Potential issue | 🔴 CriticalFix
Via.Encode()andVia.Decode()– undefinednativevariable andbinary.Writeincompatibility.
Via.Encode()fails at runtime becausebinary.Writecannot serializenetip.Addr(not fixed-size). Additionally, bothVia.Encode()andVia.Decode()reference an undefinednativevariable, which will fail to compile. Definenative := nl.NativeEndian()at the start of both methods, then serialize the address family and raw bytes manually usingv.Addr.AsSlice(), validating byte length against the family.Also fix
RouteGetWithOptions(line 1628):msg.Src_lenshould useoptions.SrcAddr.BitLen(), notdestination.BitLen().Proposed fix for Via.Encode and Via.Decode
func (v *Via) Encode() ([]byte, error) { + native := nl.NativeEndian() - buf := &bytes.Buffer{} - err := binary.Write(buf, native, uint16(v.AddrFamily)) - if err != nil { - return nil, err - } - err = binary.Write(buf, native, v.Addr) - if err != nil { - return nil, err - } - return buf.Bytes(), nil + addr := v.Addr.AsSlice() + switch v.AddrFamily { + case nl.FAMILY_V4: + if len(addr) != net.IPv4len { + return nil, fmt.Errorf("RTA_VIA expects an IPv4 address") + } + case nl.FAMILY_V6: + if len(addr) != net.IPv6len { + return nil, fmt.Errorf("RTA_VIA expects an IPv6 address") + } + default: + return nil, fmt.Errorf("RTA_VIA unsupported family %d", v.AddrFamily) + } + + buf := make([]byte, 2+len(addr)) + native.PutUint16(buf[:2], uint16(v.AddrFamily)) + copy(buf[2:], addr) + return buf, nil } func (v *Via) Decode(b []byte) error { + native := nl.NativeEndian() if len(b) < 6 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@route_linux.go` around lines 781 - 788, Define native := nl.NativeEndian() at the start of Via.Encode and Via.Decode, replace binary.Write of netip.Addr with manual serialization using v.Addr.AsSlice() (or addr.AsSlice()) and write the address family as uint16 then the raw bytes to the buffer/reader, validating the slice length matches the expected length for the AddrFamily (e.g., 4 for IPv4, 16 for IPv6) and return errors on mismatch; in Via.Decode read the family using native, read the expected number of bytes and construct the netip.Addr from the slice. Also update RouteGetWithOptions to set msg.Src_len = uint8(options.SrcAddr.BitLen()) (not destination.BitLen()) so the source prefix length is taken from options.SrcAddr.
🧹 Nitpick comments (3)
filter_linux.go (1)
1042-1045: Consider adding error handling for invalid address data.The
netip.AddrFromSlicereturn value is discarded with_. If the slice contains invalid address data,SrcAddr/DstAddrwill be set to the zero value, which will later cause an error during re-encoding at lines 750-763.For consistency with
Flower.decode(which returns an error for invalid addresses at lines 216-218, 224-226, etc.), consider propagating the error here as well.♻️ Suggested improvement
case nl.TCA_TUNNEL_KEY_ENC_IPV6_SRC, nl.TCA_TUNNEL_KEY_ENC_IPV4_SRC: - action.(*TunnelKeyAction).SrcAddr, _ = netip.AddrFromSlice(adatum.Value) + addr, ok := netip.AddrFromSlice(adatum.Value) + if !ok { + return nil, fmt.Errorf("invalid tunnel key src address") + } + action.(*TunnelKeyAction).SrcAddr = addr case nl.TCA_TUNNEL_KEY_ENC_IPV6_DST, nl.TCA_TUNNEL_KEY_ENC_IPV4_DST: - action.(*TunnelKeyAction).DstAddr, _ = netip.AddrFromSlice(adatum.Value) + addr, ok := netip.AddrFromSlice(adatum.Value) + if !ok { + return nil, fmt.Errorf("invalid tunnel key dst address") + } + action.(*TunnelKeyAction).DstAddr = addr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filter_linux.go` around lines 1042 - 1045, The switch currently discards the boolean from netip.AddrFromSlice when populating TunnelKeyAction.SrcAddr/DstAddr; change this to check the ok return value and return an error (propagate) when the slice is not a valid address instead of silently assigning the zero addr. Specifically, call netip.AddrFromSlice(adatum.Value), verify ok, only assign to action.(*TunnelKeyAction).SrcAddr or DstAddr when ok, and return a descriptive error (e.g., "invalid tunnel key address for TCA_TUNNEL_KEY_ENC_IPV6_SRC" or similar) so callers receive the failure (consistent with Flower.decode’s error propagation).conntrack_linux.go (1)
463-467: Consider handling parse failures for robustness.The parse error from
netip.AddrFromSliceis discarded. While kernel netlink data should be well-formed, silently ignoring parse failures could mask issues. The zero-valueAddr(invalid) would propagate silently.This is low risk since the data comes from the kernel, but for defensive coding, you might consider logging parse failures in debug scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conntrack_linux.go` around lines 463 - 467, The code currently discards errors from netip.AddrFromSlice when populating tpl.SrcIP and tpl.DstIP inside the CTA_IP_* case, which can hide malformed data; update the parsing to check the returned boolean/error and, on failure, leave the Addr as invalid but emit a debug log (or increment a metric) identifying the field (SrcIP or DstIP) and the offending byte slice, e.g. inside the switch case handling nl.CTA_IP_V4_SRC/nl.CTA_IP_V6_SRC and nl.CTA_IP_V4_DST/nl.CTA_IP_V6_DST, call netip.AddrFromSlice, test the ok flag, assign only on success and log a debug message including the slice and which tpl field failed to parse so failures are visible in debug builds.route_test.go (1)
2253-2261: Add a mapped-IPv6 next-hop regression here.This test only covers a normal IPv6
Via(2001::1), so it doesn’t lock down the bug from#1114. Please add a case that uses an IPv4-mapped IPv6 next hop like::ffff:100.95.128.2here (or in an equivalentGwtest) to prove the family detection no longer collapses it to IPv4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@route_test.go` around lines 2253 - 2261, Add a regression test entry that uses an IPv4-mapped IPv6 next-hop to ensure family detection stays IPv6: modify the Route test case that constructs route := &Route{... MultiPath: []*NexthopInfo{{ LinkIndex: link.Attrs().Index, Via: &Via{ AddrFamily: FAMILY_V6, Addr: netip.MustParseAddr("::ffff:100.95.128.2"), ... } } }} (or add an equivalent Gw test) and assert that the Via.AddrFamily remains FAMILY_V6 and the Addr parses as the mapped IPv6 address, so the code no longer collapses mapped-IPv6 to IPv4.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@route_linux.go`:
- Around line 1627-1629: The code sets msg.Src_len using destination.BitLen()
but should derive it from the source address; change the assignment so that when
options != nil and options.SrcAddr.IsValid() you set msg.Src_len =
uint8(options.SrcAddr.BitLen()) (i.e., use options.SrcAddr.BitLen() instead of
destination.BitLen()) to serialize the correct source prefix length.
---
Outside diff comments:
In `@nl/tc_linux.go`:
- Around line 1626-1656: The IPv4 setter functions SetIPv4Src and SetIPv4Dst
must reject or unwrap IPv6 inputs before reading 4 bytes; call ip.Unmap() (or
use ip.As4()/ip.Is4()) and verify you have a valid IPv4 address before calling
NativeEndian().Uint32 on the byte slice. Update both TcPedit.SetIPv4Src and
TcPedit.SetIPv4Dst to obtain the 4-byte IPv4 form (e.g., via v4, ok := ip.As4()
or ip = ip.Unmap(); v4, ok := ip.As4()), return or error when ok is false, then
use that 4-byte value to populate tKey.Val and proceed as before.
In `@route_linux.go`:
- Around line 781-788: Define native := nl.NativeEndian() at the start of
Via.Encode and Via.Decode, replace binary.Write of netip.Addr with manual
serialization using v.Addr.AsSlice() (or addr.AsSlice()) and write the address
family as uint16 then the raw bytes to the buffer/reader, validating the slice
length matches the expected length for the AddrFamily (e.g., 4 for IPv4, 16 for
IPv6) and return errors on mismatch; in Via.Decode read the family using native,
read the expected number of bytes and construct the netip.Addr from the slice.
Also update RouteGetWithOptions to set msg.Src_len =
uint8(options.SrcAddr.BitLen()) (not destination.BitLen()) so the source prefix
length is taken from options.SrcAddr.
---
Nitpick comments:
In `@conntrack_linux.go`:
- Around line 463-467: The code currently discards errors from
netip.AddrFromSlice when populating tpl.SrcIP and tpl.DstIP inside the CTA_IP_*
case, which can hide malformed data; update the parsing to check the returned
boolean/error and, on failure, leave the Addr as invalid but emit a debug log
(or increment a metric) identifying the field (SrcIP or DstIP) and the offending
byte slice, e.g. inside the switch case handling
nl.CTA_IP_V4_SRC/nl.CTA_IP_V6_SRC and nl.CTA_IP_V4_DST/nl.CTA_IP_V6_DST, call
netip.AddrFromSlice, test the ok flag, assign only on success and log a debug
message including the slice and which tpl field failed to parse so failures are
visible in debug builds.
In `@filter_linux.go`:
- Around line 1042-1045: The switch currently discards the boolean from
netip.AddrFromSlice when populating TunnelKeyAction.SrcAddr/DstAddr; change this
to check the ok return value and return an error (propagate) when the slice is
not a valid address instead of silently assigning the zero addr. Specifically,
call netip.AddrFromSlice(adatum.Value), verify ok, only assign to
action.(*TunnelKeyAction).SrcAddr or DstAddr when ok, and return a descriptive
error (e.g., "invalid tunnel key address for TCA_TUNNEL_KEY_ENC_IPV6_SRC" or
similar) so callers receive the failure (consistent with Flower.decode’s error
propagation).
In `@route_test.go`:
- Around line 2253-2261: Add a regression test entry that uses an IPv4-mapped
IPv6 next-hop to ensure family detection stays IPv6: modify the Route test case
that constructs route := &Route{... MultiPath: []*NexthopInfo{{ LinkIndex:
link.Attrs().Index, Via: &Via{ AddrFamily: FAMILY_V6, Addr:
netip.MustParseAddr("::ffff:100.95.128.2"), ... } } }} (or add an equivalent Gw
test) and assert that the Via.AddrFamily remains FAMILY_V6 and the Addr parses
as the mapped IPv6 address, so the code no longer collapses mapped-IPv6 to IPv4.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0d16dbf-24c6-4b6b-85f1-d93bed1e92ad
📒 Files selected for processing (20)
addr.goaddr_test.gobridge_linux_test.goconntrack_linux.goconntrack_test.gofilter_linux.golink_linux.gonetlink.gonexthop.gonexthop_linux.gonexthop_test.gonl/tc_linux.gonl/tc_linux_test.gonl/xfrm_linux.goroute_linux.goroute_test.gorule_linux.goxfrm_policy_linux.goxfrm_policy_linux_test.goxfrm_state_linux.go
✅ Files skipped from review due to trivial changes (2)
- bridge_linux_test.go
- nexthop_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- nexthop.go
- nexthop_linux.go
- nl/tc_linux_test.go
- addr_test.go
- netlink.go
- xfrm_policy_linux_test.go
- rule_linux.go
- link_linux.go
- nl/xfrm_linux.go
| if options != nil && options.SrcAddr.IsValid() { | ||
| msg.Src_len = uint8(destination.BitLen()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "route_linux.go" -type fRepository: vishvananda/netlink
Length of output: 99
🏁 Script executed:
sed -n '1620,1640p' route_linux.goRepository: vishvananda/netlink
Length of output: 662
🏁 Script executed:
sed -n '1600,1650p' route_linux.goRepository: vishvananda/netlink
Length of output: 1672
🏁 Script executed:
sed -n '1590,1610p' route_linux.goRepository: vishvananda/netlink
Length of output: 618
🏁 Script executed:
sed -n '1630,1670p' route_linux.goRepository: vishvananda/netlink
Length of output: 938
🏁 Script executed:
sed -n '1670,1710p' route_linux.goRepository: vishvananda/netlink
Length of output: 951
🏁 Script executed:
grep -n "msg.Src_len" route_linux.goRepository: vishvananda/netlink
Length of output: 111
🏁 Script executed:
grep -n "Src_len\|Dst_len" route_linux.go | head -20Repository: vishvananda/netlink
Length of output: 332
Set msg.Src_len from the source address, not the destination.
Line 1628 incorrectly uses destination.BitLen() for msg.Src_len. When options.SrcAddr is set, its prefix length should be used, not the destination's. This causes the serialized source prefix length to be wrong when the source and destination have different widths.
Proposed fix
if options != nil && options.SrcAddr.IsValid() {
- msg.Src_len = uint8(destination.BitLen())
+ msg.Src_len = uint8(options.SrcAddr.BitLen())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if options != nil && options.SrcAddr.IsValid() { | |
| msg.Src_len = uint8(destination.BitLen()) | |
| } | |
| if options != nil && options.SrcAddr.IsValid() { | |
| msg.Src_len = uint8(options.SrcAddr.BitLen()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@route_linux.go` around lines 1627 - 1629, The code sets msg.Src_len using
destination.BitLen() but should derive it from the source address; change the
assignment so that when options != nil and options.SrcAddr.IsValid() you set
msg.Src_len = uint8(options.SrcAddr.BitLen()) (i.e., use
options.SrcAddr.BitLen() instead of destination.BitLen()) to serialize the
correct source prefix length.
There was a problem hiding this comment.
The logic has been kept. Using either destination or source doesn't change anything, since they're both supposed to have the same IP family.
There was a problem hiding this comment.
@fdomain, that's a fair point. Since SrcAddr and destination must share the same IP family, destination.BitLen() and options.SrcAddr.BitLen() will always yield the same value (32 for IPv4, 128 for IPv6). The original net.IP code also derived a single bitlen from the destination and reused it for both Dst_len and Src_len, so the intent is consistent. I'll withdraw the suggestion.
✏️ Learnings added
Learnt from: fdomain
Repo: vishvananda/netlink PR: 1181
File: route_linux.go:1626-1628
Timestamp: 2026-04-13T19:44:56.939Z
Learning: In route_linux.go (vishvananda/netlink), `msg.Src_len` in `RouteGetWithOptions` is intentionally set using `destination.BitLen()` rather than `options.SrcAddr.BitLen()`. This is correct because SrcAddr and destination are always expected to be the same IP family (both IPv4 = 32 bits, or both IPv6 = 128 bits), so both expressions yield the same value. The original net.IP implementation also used a single bitlen variable for both Dst_len and Src_len.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kyrtapz
Repo: vishvananda/netlink PR: 1159
File: bridge_linux.go:349-366
Timestamp: 2026-02-06T09:13:09.289Z
Learning: VNI filter operations (RTM_NEWTUNNEL, RTM_DELTUNNEL, RTM_GETTUNNEL) in bridge_linux.go do not require NLM_F_CREATE or NLM_F_APPEND flags. They only need NLM_F_ACK for acknowledgment, as confirmed by the iproute2 reference implementation in bridge/vni.c.
Learnt from: mattedallo
Repo: vishvananda/netlink PR: 1164
File: bridge_linux.go:44-48
Timestamp: 2026-02-09T08:32:40.092Z
Learning: In bridge_linux.go, BridgeVlanTunnelShow and BridgeVlanTunnelShowDev correctly use RTEXT_FILTER_BRVLAN (not RTEXT_FILTER_BRVLAN_COMPRESSED) for tunnel info dumps, matching iproute2's bridge vlan tunnelshow default behavior. RTEXT_FILTER_BRVLAN_COMPRESSED is only used with the -compressvlans flag to control VLAN range compression in display output, not tunnel info availability.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nl/tc_linux.go (1)
1626-1644:⚠️ Potential issue | 🟠 MajorUse
Unmap()before encoding IPv4 fields inSetIPv4SrcandSetIPv4Dst.Lines 1627 and 1643 use
ip.AsSlice()directly on anetip.Addrargument. For IPv4-mapped IPv6 addresses (e.g.,::ffff:192.0.2.1),AsSlice()returns the 16-byte IPv6 representation, so reading the first 4 bytes encodes the wrong data. Plain IPv6 addresses also get silently mis-encoded instead of being rejected. The previousnet.IP.To4()behavior collapsed mapped IPv6 to their embedded IPv4, which this implementation loses.Suggested fix
func (p *TcPedit) SetIPv4Src(ip netip.Addr) { - u32 := NativeEndian().Uint32(ip.AsSlice()) + ip = ip.Unmap() + if !ip.Is4() { + return + } + ip4 := ip.As4() + u32 := NativeEndian().Uint32(ip4[:]) @@ func (p *TcPedit) SetIPv4Dst(ip netip.Addr) { - u32 := NativeEndian().Uint32(ip.AsSlice()) + ip = ip.Unmap() + if !ip.Is4() { + return + } + ip4 := ip.As4() + u32 := NativeEndian().Uint32(ip4[:])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nl/tc_linux.go` around lines 1626 - 1644, The SetIPv4Src and SetIPv4Dst methods must call ip = ip.Unmap() and validate that the result is an IPv4 address before extracting bytes: unmap the incoming netip.Addr to collapse IPv4-mapped IPv6, check ip.Is4() (and return/handle error or skip if not IPv4), then extract the 4-byte IPv4 value (use the 4-byte view/As4()/AsSlice() after Unmap) and use that uint32 when populating TcPeditKey.Val; update both SetIPv4Src and SetIPv4Dst accordingly to avoid encoding 16-byte IPv6 representations.
🧹 Nitpick comments (1)
nexthop_test.go (1)
55-60: Add a mapped-IPv6 gateway case here.This only exercises a pure IPv6 gateway and a pure IPv4 gateway. The regression in issue
#1114is::ffff:x.y.z.wbeing treated as IPv4, so this test can still pass while the original bug remains untested. Please add one case with something likenetip.MustParseAddr("::ffff:100.95.128.2").Also applies to: 123-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nexthop_test.go` around lines 55 - 60, Add a mapped-IPv6 gateway test case to cover the ::ffff:IPv4-mapped address scenario: create an additional Nexthop instance (similar to nh1/nh2) with Gateway set to netip.MustParseAddr("::ffff:100.95.128.2") and the same fields (ID, OIF, Protocol) used in the other cases, then assert expected behavior in the same way the test asserts for pure IPv4/IPv6 gateways; update any table-driven test entries (lines around the nh1/nh2 cases) to include this new case so the code handling Nexthop/Gateway correctly treats mapped IPv6 addresses as IPv6-mapped addresses rather than plain IPv4.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@filter_linux.go`:
- Around line 57-64: The mask decoding currently uses maskLen to count set bits
but doesn't verify contiguity, allowing non-CIDR masks to be accepted; update
maskLen (or the netlink decode path that converts masks to netip.Prefix) to
validate that all 1 bits are contiguous (i.e., once a 0 bit is seen no
subsequent 1 bits are allowed) and return an error if the mask is
non-contiguous, then propagate that error from the decoder so callers
constructing netip.Prefix (e.g., in the functions handling
EncSrc/EncDest/Src/Dest decoding) reject invalid masks instead of silently
converting them to a shorter prefix length.
In `@nl/xfrm_linux.go`:
- Around line 159-168: The invalid-input branch in XfrmAddress.FromIP currently
only clears x[4:16], leaving x[0:4] stale and leaking previous IPv4 bytes;
update the invalid case in the FromIP method to zero the entire XfrmAddress (all
16 bytes, e.g., copy(x[0:16], empty[:]) or equivalent) so a reused XfrmAddress
does not retain previous data.
In `@route_test.go`:
- Around line 2253-2262: Add a regression test case in route_test.go alongside
the existing Route/NexthopInfo/Via test that uses an IPv4-mapped IPv6 gateway
address to exercise the previously failing path; specifically, create a Route
with MultiPath containing a NexthopInfo whose Via has AddrFamily FAMILY_V6 but
Addr set to an IPv4-mapped IPv6 address (e.g. ::ffff:100.95.128.2) and assert
the same expected behavior/translation as the native-IPv6 case so the bug in
`#1114` cannot regress (look for the Route, NexthopInfo, Via, and FAMILY_V6
identifiers to place the new case).
---
Outside diff comments:
In `@nl/tc_linux.go`:
- Around line 1626-1644: The SetIPv4Src and SetIPv4Dst methods must call ip =
ip.Unmap() and validate that the result is an IPv4 address before extracting
bytes: unmap the incoming netip.Addr to collapse IPv4-mapped IPv6, check
ip.Is4() (and return/handle error or skip if not IPv4), then extract the 4-byte
IPv4 value (use the 4-byte view/As4()/AsSlice() after Unmap) and use that uint32
when populating TcPeditKey.Val; update both SetIPv4Src and SetIPv4Dst
accordingly to avoid encoding 16-byte IPv6 representations.
---
Nitpick comments:
In `@nexthop_test.go`:
- Around line 55-60: Add a mapped-IPv6 gateway test case to cover the
::ffff:IPv4-mapped address scenario: create an additional Nexthop instance
(similar to nh1/nh2) with Gateway set to
netip.MustParseAddr("::ffff:100.95.128.2") and the same fields (ID, OIF,
Protocol) used in the other cases, then assert expected behavior in the same way
the test asserts for pure IPv4/IPv6 gateways; update any table-driven test
entries (lines around the nh1/nh2 cases) to include this new case so the code
handling Nexthop/Gateway correctly treats mapped IPv6 addresses as IPv6-mapped
addresses rather than plain IPv4.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13e484df-ceda-44e0-9658-34f05edcb99a
📒 Files selected for processing (24)
addr.goaddr_test.gobridge_linux_test.goconntrack_linux.goconntrack_test.gofilter_linux.gofilter_test.golink_linux.gonetlink.gonexthop.gonexthop_linux.gonexthop_test.gonl/tc_linux.gonl/tc_linux_test.gonl/xfrm_linux.goroute_linux.goroute_test.gorule_linux.gorule_test.gosocket_linux.gosocket_test.goxfrm_policy_linux.goxfrm_policy_linux_test.goxfrm_state_linux.go
✅ Files skipped from review due to trivial changes (4)
- bridge_linux_test.go
- socket_test.go
- addr_test.go
- rule_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- nexthop.go
- nl/tc_linux_test.go
- rule_linux.go
- socket_linux.go
- xfrm_policy_linux_test.go
- link_linux.go
- route_linux.go
| type Flower struct { | ||
| FilterAttrs | ||
| ClassId uint32 | ||
| DestIP net.IP | ||
| DestIPMask net.IPMask | ||
| SrcIP net.IP | ||
| SrcIPMask net.IPMask | ||
| Dest netip.Prefix | ||
| Src netip.Prefix | ||
| EthType uint16 | ||
| EncDestIP net.IP | ||
| EncDestIPMask net.IPMask | ||
| EncSrcIP net.IP | ||
| EncSrcIPMask net.IPMask | ||
| EncDest netip.Prefix | ||
| EncSrc netip.Prefix |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can Go's net/netip.Prefix represent non-contiguous IP masks, or only contiguous CIDR prefixes?
💡 Result:
No, Go's net/netip.Prefix can only represent contiguous CIDR prefixes, not non-contiguous IP masks.
Citations:
- 1: https://pkg.go.dev/net/netip@go1.25.6
- 2: https://pkg.go.dev/net/netip@master
- 3: https://pkg.go.dev/net/netip
- 4: https://go.pact.im/doc/src/net/netip/netip.go.html
- 5: https://www.ripe.net/participate/ripe
- 6: https://tip.golang.org/src/net/netip/netip.go
🏁 Script executed:
fd -n "filter_linux.go" --type fRepository: vishvananda/netlink
Length of output: 293
🏁 Script executed:
fd "filter_linux.go" --type fRepository: vishvananda/netlink
Length of output: 78
🏁 Script executed:
wc -l filter_linux.goRepository: vishvananda/netlink
Length of output: 83
🏁 Script executed:
sed -n '57,64p' filter_linux.goRepository: vishvananda/netlink
Length of output: 264
🏁 Script executed:
sed -n '203,248p' filter_linux.goRepository: vishvananda/netlink
Length of output: 1613
🏁 Script executed:
sed -n '300,327p' filter_linux.goRepository: vishvananda/netlink
Length of output: 514
🏁 Script executed:
sed -n '180,210p' filter_linux.goRepository: vishvananda/netlink
Length of output: 798
🏁 Script executed:
sed -n '100,180p' filter_linux.goRepository: vishvananda/netlink
Length of output: 3041
🏁 Script executed:
sed -n '70,105p' filter_linux.goRepository: vishvananda/netlink
Length of output: 822
Validate CIDR contiguity when decoding masks, or reject non-CIDR masks explicitly.
The maskLen function naively counts set bits without validating contiguity. When decoding netlink attributes, a non-CIDR mask like ff00ff00 would be decoded as /16 instead of being rejected, causing the filter to silently interpret a mismatched mask. Since netip.Prefix only supports contiguous CIDR prefixes, the decode path should validate mask contiguity bit-by-bit or return an error for invalid masks.
Suggested fix
- maskLen := func(d []byte) (r int) {
- for _, v := range d {
- r += bits.OnesCount8(v)
- }
- return
- }
+ maskLen := func(d []byte) (int, error) {
+ n := 0
+ seenZero := false
+ for _, v := range d {
+ for bit := 7; bit >= 0; bit-- {
+ one := v&(1<<bit) != 0
+ if seenZero && one {
+ return 0, fmt.Errorf("non-CIDR mask")
+ }
+ if one {
+ n++
+ } else {
+ seenZero = true
+ }
+ }
+ }
+ return n, nil
+ }Applies to lines 203–248, 300–327.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@filter_linux.go` around lines 57 - 64, The mask decoding currently uses
maskLen to count set bits but doesn't verify contiguity, allowing non-CIDR masks
to be accepted; update maskLen (or the netlink decode path that converts masks
to netip.Prefix) to validate that all 1 bits are contiguous (i.e., once a 0 bit
is seen no subsequent 1 bits are allowed) and return an error if the mask is
non-contiguous, then propagate that error from the decoder so callers
constructing netip.Prefix (e.g., in the functions handling
EncSrc/EncDest/Src/Dest decoding) reject invalid masks instead of silently
converting them to a shorter prefix length.
There was a problem hiding this comment.
We're not supposed to receive invalid CIDR mask from netlink. And there was no "contiguity" check before.
There was a problem hiding this comment.
@fdomain, that's a fair point — the Linux kernel always emits well-formed CIDR masks over netlink, so there's no real risk of receiving a non-contiguous mask in practice, and the pre-migration code had no such check either. I'll drop this suggestion.
✏️ Learnings added
Learnt from: fdomain
Repo: vishvananda/netlink PR: 1181
File: filter_linux.go:57-64
Timestamp: 2026-04-13T19:48:23.020Z
Learning: In vishvananda/netlink (filter_linux.go), the `maskLen` helper used in `Flower.decode` does not need to validate CIDR contiguity. The Linux kernel always sends valid contiguous CIDR masks over netlink, and the original `net.IP`/`net.IPMask`-based implementation had no such check either. Do not flag the absence of contiguity validation in netlink mask decoding as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: fdomain
Repo: vishvananda/netlink PR: 1181
File: route_linux.go:1626-1628
Timestamp: 2026-04-13T19:44:56.939Z
Learning: In route_linux.go (vishvananda/netlink), `msg.Src_len` in `RouteGetWithOptions` is intentionally set using `destination.BitLen()` rather than `options.SrcAddr.BitLen()`. This is correct because SrcAddr and destination are always expected to be the same IP family (both IPv4 = 32 bits, or both IPv6 = 128 bits), so both expressions yield the same value. The original net.IP implementation also used a single bitlen variable for both Dst_len and Src_len.
Learnt from: kyrtapz
Repo: vishvananda/netlink PR: 1159
File: bridge_linux.go:349-366
Timestamp: 2026-02-06T09:13:09.289Z
Learning: VNI filter operations (RTM_NEWTUNNEL, RTM_DELTUNNEL, RTM_GETTUNNEL) in bridge_linux.go do not require NLM_F_CREATE or NLM_F_APPEND flags. They only need NLM_F_ACK for acknowledgment, as confirmed by the iproute2 reference implementation in bridge/vni.c.
Learnt from: mattedallo
Repo: vishvananda/netlink PR: 1164
File: bridge_linux.go:44-48
Timestamp: 2026-02-09T08:32:40.092Z
Learning: In bridge_linux.go, BridgeVlanTunnelShow and BridgeVlanTunnelShowDev correctly use RTEXT_FILTER_BRVLAN (not RTEXT_FILTER_BRVLAN_COMPRESSED) for tunnel info dumps, matching iproute2's bridge vlan tunnelshow default behavior. RTEXT_FILTER_BRVLAN_COMPRESSED is only used with the -compressvlans flag to control VLAN range compression in display output, not tunnel info availability.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
socket_linux.go (1)
276-290:⚠️ Potential issue | 🟠 Major
SocketDestroystill uses the oldTo4()classification path.Lines 276-290 force IPv6 through
To4()and hard-codeFamily: unix.AF_INET, leaving a family-mismatch issue. The parallelSocketGetfunction correctly handles this using.Unmap()and dynamic family detection (Is4()/Is6()). Apply the same pattern here: either derive the family dynamically or reject non-IPv4 addresses and document as IPv4-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@socket_linux.go` around lines 276 - 290, SocketDestroy currently forces IPv4 by calling To4() on localTCP.IP/remoteTCP.IP and hard-coding Family: unix.AF_INET, causing family mismatches; change it to follow the SocketGet pattern: use .Unmap() on the IPs and detect family with addr.Is4()/Is6(), set socketRequest.Family dynamically (unix.AF_INET or unix.AF_INET6) and construct netip.AddrFromSlice from the appropriate 4- or 16-byte slice, or explicitly return an error if you intend to only support IPv4; update handling of localIP/remoteIP and Family in the socketRequest creation accordingly.
♻️ Duplicate comments (2)
nl/xfrm_linux.go (1)
159-168:⚠️ Potential issue | 🟠 MajorZero the entire
XfrmAddresson invalid input.The invalid branch only clears
x[4:16]. If this struct is reused after an IPv4 write,x[0:4]keeps stale bytes and the reset leaks the previous address.Proposed fix
func (x *XfrmAddress) FromIP(ip netip.Addr) { - var empty = [16]byte{} + *x = XfrmAddress{} if !ip.IsValid() { - copy(x[4:16], empty[:]) + return } else if ip.Is4() { copy(x[0:4], ip.AsSlice()) - copy(x[4:16], empty[:12]) } else { copy(x[0:16], ip.AsSlice()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nl/xfrm_linux.go` around lines 159 - 168, The FromIP method on XfrmAddress leaves bytes 0:4 intact when ip is invalid, so stale IPv4 data can leak; update XfrmAddress.FromIP to zero the entire receiver when ip.IsValid() is false (e.g., copy a zeroed 16-byte array into x[:]) and keep the current behavior for ip.Is4() (write first 4 bytes then zero the remaining 12) and non-IPv4 (copy all 16 bytes) to ensure no stale bytes remain.filter_linux.go (1)
203-208:⚠️ Potential issue | 🟠 MajorReject non-CIDR masks before converting them to
netip.Prefix.
maskLenjust counts set bits. A mask likeff00ff00becomes/16, butnetip.Prefixcan only represent contiguous CIDR masks, so the decode path silently changes the filter the kernel returned.Proposed fix
- maskLen := func(d []byte) (r int) { - for _, v := range d { - r += bits.OnesCount8(v) - } - return - } + maskLen := func(d []byte) (int, error) { + n := 0 + seenZero := false + for _, v := range d { + for bit := 7; bit >= 0; bit-- { + one := v&(1<<bit) != 0 + if seenZero && one { + return 0, fmt.Errorf("non-CIDR flower mask") + } + if one { + n++ + } else { + seenZero = true + } + } + } + return n, nil + }Then propagate the returned error at Lines 221, 230, 239, and 248 instead of accepting the decoded mask unconditionally.
Can Go's net/netip.Prefix represent non-contiguous IP masks, or only contiguous CIDR prefixes?Also applies to: 220-248, 300-327
🧹 Nitpick comments (2)
rule_linux.go (1)
380-384: Update stale documentation comment.The comment still references
*net.IPNetand "nil IPNet" but the function now usesnetip.Prefix. Consider updating for accuracy.📝 Suggested documentation update
-// ruleIPNetEqual compares two *net.IPNet for rule filtering purposes. -// Unlike ipNetEqual in route.go, this treats a nil IPNet as equivalent +// ruleIPNetEqual compares two netip.Prefix for rule filtering purposes. +// Unlike ipNetEqual in route.go, this treats an invalid Prefix as equivalent // to 0.0.0.0/0 or ::/0, which is how the kernel represents "from all" / // "to all" rules (it omits FRA_SRC/FRA_DST when prefix length is 0). // Two /0 prefixes are always equal regardless of IP (e.g. 0.0.0.0/0 == 1.2.3.4/0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rule_linux.go` around lines 380 - 384, The doc comment for ruleIPNetEqual is stale: it refers to *net.IPNet and "nil IPNet" but the function now uses netip.Prefix; update the comment to accurately describe that ruleIPNetEqual compares netip.Prefix values, treats an invalid/zero-value netip.Prefix (or a prefix with bits == 0) as the kernel's "from all"/"to all" (/0) representation, and that any two /0 prefixes are considered equal regardless of underlying address; keep the examples and intent the same but replace mentions of nil/*net.IPNet with netip.Prefix and invalid/zero-value prefix semantics and reference the function name ruleIPNetEqual.addr_test.go (1)
237-246: Avoid per-iteration timeout allocation inexpectAddrUpdate.
time.After(time.Minute)is recreated on every loop pass. Hoisting it outside the loop avoids repeated timer allocation while preserving behavior.♻️ Proposed refactor
func expectAddrUpdate(ch <-chan AddrUpdate, add bool, dst netip.Addr) bool { + timeout := time.After(time.Minute) for { - timeout := time.After(time.Minute) select { case update := <-ch: if update.NewAddr == add && update.LinkAddress.Addr() == dst { return true } case <-timeout: return false } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@addr_test.go` around lines 237 - 246, The loop in expectAddrUpdate recreates time.After(time.Minute) on every iteration; move the timeout channel creation out of the loop to avoid per-iteration timer allocation: before the for { ... } create a single timeoutCh := time.After(time.Minute) (or a time.NewTimer and defer timer.Stop() if you need to stop/reset) and then use case <-timeoutCh: inside the select, keeping the existing checks against update.NewAddr and update.LinkAddress.Addr() == dst in the expectAddrUpdate function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nl/xfrm_linux.go`:
- Around line 144-156: The current ToPrefix falls back to x.ToIP() for an
all-zero buffer which decodes as 0.0.0.0 and breaks IPv6 selectors (e.g. ::/64);
change the empty-buffer branch to switch on family and return
netip.PrefixFrom(v6zero, int(prefixlen)) for FAMILY_V6 (and
netip.PrefixFrom(v4zero, int(prefixlen)) for FAMILY_V4) instead of falling
through to x.ToIP(); update the XfrmAddress.ToPrefix function to handle the
all-zero case by returning the appropriate zero address prefix based on family.
In `@socket_linux.go`:
- Around line 187-216: SocketGet is collapsing IPv6-mapped IPv4 by calling
Unmap() on addresses; remove the Unmap() calls in the TCP and UDP branches (the
spots that set localIP and remoteIP) so you preserve the original netip.Addr
representation, keep the rest of the assignments (localPort/remotePort/protocol)
the same, and let the existing family detection (the checks that set family to
unix.AF_INET or unix.AF_INET6) operate on the unmodified netip.Addr values;
alternatively consider changing the API to accept netip.Addr directly instead of
net.TCPAddr/net.UDPAddr to avoid losing family semantics.
---
Outside diff comments:
In `@socket_linux.go`:
- Around line 276-290: SocketDestroy currently forces IPv4 by calling To4() on
localTCP.IP/remoteTCP.IP and hard-coding Family: unix.AF_INET, causing family
mismatches; change it to follow the SocketGet pattern: use .Unmap() on the IPs
and detect family with addr.Is4()/Is6(), set socketRequest.Family dynamically
(unix.AF_INET or unix.AF_INET6) and construct netip.AddrFromSlice from the
appropriate 4- or 16-byte slice, or explicitly return an error if you intend to
only support IPv4; update handling of localIP/remoteIP and Family in the
socketRequest creation accordingly.
---
Duplicate comments:
In `@nl/xfrm_linux.go`:
- Around line 159-168: The FromIP method on XfrmAddress leaves bytes 0:4 intact
when ip is invalid, so stale IPv4 data can leak; update XfrmAddress.FromIP to
zero the entire receiver when ip.IsValid() is false (e.g., copy a zeroed 16-byte
array into x[:]) and keep the current behavior for ip.Is4() (write first 4 bytes
then zero the remaining 12) and non-IPv4 (copy all 16 bytes) to ensure no stale
bytes remain.
---
Nitpick comments:
In `@addr_test.go`:
- Around line 237-246: The loop in expectAddrUpdate recreates
time.After(time.Minute) on every iteration; move the timeout channel creation
out of the loop to avoid per-iteration timer allocation: before the for { ... }
create a single timeoutCh := time.After(time.Minute) (or a time.NewTimer and
defer timer.Stop() if you need to stop/reset) and then use case <-timeoutCh:
inside the select, keeping the existing checks against update.NewAddr and
update.LinkAddress.Addr() == dst in the expectAddrUpdate function.
In `@rule_linux.go`:
- Around line 380-384: The doc comment for ruleIPNetEqual is stale: it refers to
*net.IPNet and "nil IPNet" but the function now uses netip.Prefix; update the
comment to accurately describe that ruleIPNetEqual compares netip.Prefix values,
treats an invalid/zero-value netip.Prefix (or a prefix with bits == 0) as the
kernel's "from all"/"to all" (/0) representation, and that any two /0 prefixes
are considered equal regardless of underlying address; keep the examples and
intent the same but replace mentions of nil/*net.IPNet with netip.Prefix and
invalid/zero-value prefix semantics and reference the function name
ruleIPNetEqual.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8f6731a-3a2b-40e8-bd41-e6073b077ac9
📒 Files selected for processing (25)
addr.goaddr_test.gobridge_linux_test.goconntrack_linux.goconntrack_test.gofilter_linux.gofilter_test.golink.golink_linux.gonetlink.gonexthop.gonexthop_linux.gonexthop_test.gonl/tc_linux.gonl/tc_linux_test.gonl/xfrm_linux.goroute_linux.goroute_test.gorule_linux.gorule_test.gosocket_linux.gosocket_test.goxfrm_policy_linux.goxfrm_policy_linux_test.goxfrm_state_linux.go
✅ Files skipped from review due to trivial changes (5)
- bridge_linux_test.go
- nexthop.go
- nexthop_test.go
- socket_test.go
- route_linux.go
🚧 Files skipped from review as they are similar to previous changes (7)
- nl/tc_linux_test.go
- nexthop_linux.go
- xfrm_policy_linux_test.go
- rule_test.go
- conntrack_test.go
- link.go
- route_test.go
| localIP, _ = netip.AddrFromSlice(l.IP) | ||
| localIP = localIP.Unmap() | ||
| localPort = uint16(l.Port) | ||
| remoteIP = r.IP | ||
| remoteIP, _ = netip.AddrFromSlice(r.IP) | ||
| remoteIP = remoteIP.Unmap() | ||
| remotePort = uint16(r.Port) | ||
| protocol = unix.IPPROTO_TCP | ||
| case *net.UDPAddr: | ||
| r, ok := remote.(*net.UDPAddr) | ||
| if !ok { | ||
| return nil, ErrNotImplemented | ||
| } | ||
| localIP = l.IP | ||
| localIP, _ = netip.AddrFromSlice(l.IP) | ||
| localIP = localIP.Unmap() | ||
| localPort = uint16(l.Port) | ||
| remoteIP = r.IP | ||
| remoteIP, _ = netip.AddrFromSlice(r.IP) | ||
| remoteIP = remoteIP.Unmap() | ||
| remotePort = uint16(r.Port) | ||
| protocol = unix.IPPROTO_UDP | ||
| default: | ||
| return nil, ErrNotImplemented | ||
| } | ||
|
|
||
| var family uint8 | ||
| if localIP.To4() != nil && remoteIP.To4() != nil { | ||
| if localIP.Is4() && remoteIP.Is4() { | ||
| family = unix.AF_INET | ||
| } | ||
|
|
||
| if family == 0 && localIP.To16() != nil && remoteIP.To16() != nil { | ||
| if family == 0 && localIP.Is6() && remoteIP.Is6() { | ||
| family = unix.AF_INET6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read socket_linux.go
find . -name "socket_linux.go" -type fRepository: vishvananda/netlink
Length of output: 80
🏁 Script executed:
# Read the socket_linux.go file to see the code at the specified lines
cat -n socket_linux.go | head -220 | tail -50Repository: vishvananda/netlink
Length of output: 1730
🏁 Script executed:
# Let me understand the netip.Addr.Unmap() behavior and look at how Socket is used
# First, check the Socket struct and its deserialize method
cat -n socket_linux.go | grep -A 30 "type Socket struct"Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
# Search for the Socket struct definition
rg "type Socket" socket_linux.go -A 10Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
# Let's see the full socket_linux.go file structure
wc -l socket_linux.goRepository: vishvananda/netlink
Length of output: 82
🏁 Script executed:
# Look at the imports and early structure
cat -n socket_linux.go | head -50Repository: vishvananda/netlink
Length of output: 1277
🏁 Script executed:
# Let's check what comes after line 220 in SocketGet to understand the complete picture
cat -n socket_linux.go | sed -n '220,240p'Repository: vishvananda/netlink
Length of output: 809
🏁 Script executed:
# Let's look at the Socket type and see how it's deserialized
rg "func.*deserialize|type Socket|SocketID" socket_linux.go | head -20Repository: vishvananda/netlink
Length of output: 340
🏁 Script executed:
# Let's find the Socket type definition
rg "^type Socket " socket_linux.go -A 20Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
# Search for Socket struct
cat -n socket_linux.go | grep -B 5 "func.*Socket.*deserialize"Repository: vishvananda/netlink
Length of output: 390
🏁 Script executed:
# Let's search more broadly for Socket type definition
rg "^type.*Socket" socket_linux.go -B 2 -A 15Repository: vishvananda/netlink
Length of output: 499
🏁 Script executed:
# Let's search in a broader way for all type definitions
rg "^type" socket_linux.go | head -30Repository: vishvananda/netlink
Length of output: 173
🏁 Script executed:
# Socket might be defined in a different file. Let's search the repo
find . -name "*.go" -type f | xargs grep -l "^type Socket struct" 2>/dev/nullRepository: vishvananda/netlink
Length of output: 74
🏁 Script executed:
# Look at the Socket type and its deserialize method
cat -n socket.go | head -100Repository: vishvananda/netlink
Length of output: 2936
🏁 Script executed:
# Now let's look at the deserialize method in socket_linux.go
cat -n socket_linux.go | sed -n '109,156p'Repository: vishvananda/netlink
Length of output: 1728
🌐 Web query:
netip.Addr.Unmap() Go documentation behavior IPv4-mapped IPv6
💡 Result:
The netip.Addr.Unmap method in Go's net/netip package removes the IPv4-mapped IPv6 address prefix (::ffff:0:0/96) from an IPv6 address if present, returning the underlying IPv4 address as a netip.Addr. For non IPv4-mapped IPv6 addresses (pure IPv4 or IPv6), it returns the address unmodified. Signature: func (ip Addr) Unmap Addr Behavior: - If ip.Is4In6 is true (IPv4-mapped IPv6 per RFC 4291), it sets the internal zone to IPv4 mode (z4), effectively converting to IPv4 Addr. - Otherwise, returns ip unchanged. Example from source and docs: - "::ffff:192.0.2.1".Unmap -> "192.0.2.1" (IPv4 Addr) - "2001:db8::1".Unmap -> "2001:db8::1" (unchanged IPv6 Addr) - "192.0.2.1".Unmap -> "192.0.2.1" (unchanged IPv4 Addr) Source code (netip.go): func (ip Addr) Unmap Addr { if ip.Is4In6 { ip.z = z4 } return ip } Related methods: - Is4 returns false for IPv4-mapped IPv6; use Unmap.Is4 to check underlying IPv4. - Is4In6 detects IPv4-mapped IPv6 addresses. - AddrFrom16 leaves IPv4-mapped as IPv6; use Unmap to convert. This behavior is consistent across Go 1.18+ (pkg.go.dev/net/netip, tip.golang.org/src/net/netip/netip.go).
Citations:
- 1: https://pkg.go.dev/net/netip
- 2: https://pkg.go.dev/net/netip@go1.25.6
- 3: https://pkg.go.dev/net/netip@master
- 4: https://pkg.go.dev/net/netip@go1.26.0
- 5: https://tip.golang.org/src/net/netip/netip.go
- 6: https://go.dev/src/net/netip/netip.go
- 7: https://oneuptime.com/blog/post/2026-03-20-go-net-netip-ipv6-operations/view
SocketGet still collapses mapped IPv6 endpoints to IPv4.
Lines 188, 191, 200, and 203 call Unmap(), which converts IPv4-mapped IPv6 addresses (e.g., ::ffff:192.0.2.1) to plain IPv4. This causes the family check at lines 211-216 to select AF_INET instead of AF_INET6, so the lookup request goes out with the wrong address family. This defeats proper support for mapped IPv6 and may cause lookups to fail if the socket was stored with an AF_INET6 family. Consider using a netip-native entry point instead of net.TCPAddr / net.UDPAddr to preserve address family semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@socket_linux.go` around lines 187 - 216, SocketGet is collapsing IPv6-mapped
IPv4 by calling Unmap() on addresses; remove the Unmap() calls in the TCP and
UDP branches (the spots that set localIP and remoteIP) so you preserve the
original netip.Addr representation, keep the rest of the assignments
(localPort/remotePort/protocol) the same, and let the existing family detection
(the checks that set family to unix.AF_INET or unix.AF_INET6) operate on the
unmodified netip.Addr values; alternatively consider changing the API to accept
netip.Addr directly instead of net.TCPAddr/net.UDPAddr to avoid losing family
semantics.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nl/tc_linux.go (1)
1626-1656:⚠️ Potential issue | 🟠 Major
SetIPv4Src/SetIPv4Dstdo not validate IPv4 input and read incorrect bytes for IPv6 addresses.Lines 1627 and 1643 call
ip.AsSlice()directly on anetip.Addrthat could be IPv6. For IPv6 (or IPv4-mapped IPv6),AsSlice()returns 16 bytes;Uint32()then consumes only the first four bytes, silently encoding the wrong address. The IPv6 setter methods (SetIPv6Src/SetIPv6Dstat lines 1491 and 1567) correctly useAs16()first. Add anIs4()check and useAs4()to extract the IPv4 bytes explicitly, matching the pattern used inSetSrcIP/SetDstIPwrapper methods.🛠️ Suggested fix
func (p *TcPedit) SetIPv4Src(ip netip.Addr) { - u32 := NativeEndian().Uint32(ip.AsSlice()) + if !ip.Is4() { + return + } + ip4 := ip.As4() + u32 := NativeEndian().Uint32(ip4[:]) tKey := TcPeditKey{} tKeyEx := TcPeditKeyEx{} @@ func (p *TcPedit) SetIPv4Dst(ip netip.Addr) { - u32 := NativeEndian().Uint32(ip.AsSlice()) + if !ip.Is4() { + return + } + ip4 := ip.As4() + u32 := NativeEndian().Uint32(ip4[:]) tKey := TcPeditKey{} tKeyEx := TcPeditKeyEx{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nl/tc_linux.go` around lines 1626 - 1656, The IPv4 setters must validate and extract IPv4 bytes explicitly: in TcPedit.SetIPv4Src and TcPedit.SetIPv4Dst check ip.Is4() and return early if false, then call ip.As4() and pass that 4-byte slice to NativeEndian().Uint32(...) (instead of ip.AsSlice()), keeping the rest of the logic (tKey.Val, tKey.Off, tKeyEx.HeaderType/Cmd, appending to p.Keys/p.KeysEx and incrementing p.Sel.NKeys) unchanged so they mirror the pattern used by SetSrcIP/SetDstIP and the IPv6 setters.
♻️ Duplicate comments (1)
rule_test.go (1)
676-679:⚠️ Potential issue | 🟡 MinorReuse
ruleIPNetEqualin this helper.
Masked().String()fixes host-bit canonicalization, but it still treats an explicit/0prefix as different from the invalid prefix the kernel returns for “from/to all”. Reusing the production helper keeps the test logic aligned withRuleListFilteredand avoids another edge-case mismatch.🔧 Suggested change
func ruleEquals(a, b Rule) bool { return a.Table == b.Table && - ((!a.Src.IsValid() && !b.Src.IsValid()) || - (a.Src.IsValid() && b.Src.IsValid() && a.Src.Masked().String() == b.Src.Masked().String())) && - ((!a.Dst.IsValid() && !b.Dst.IsValid()) || - (a.Dst.IsValid() && b.Dst.IsValid() && a.Dst.Masked().String() == b.Dst.Masked().String())) && + ruleIPNetEqual(a.Src, b.Src) && + ruleIPNetEqual(a.Dst, b.Dst) && a.OifName == b.OifName &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rule_test.go` around lines 676 - 679, The test currently compares prefixes using Masked().String(), which misses the kernel's "from/to all" invalid vs explicit /0 edge case; replace those comparisons with calls to the production helper ruleIPNetEqual to ensure canonicalization matches RuleListFiltered behavior—i.e., in the helper that checks a.Src vs b.Src and a.Dst vs b.Dst, call ruleIPNetEqual(a.Src, b.Src) and ruleIPNetEqual(a.Dst, b.Dst) instead of comparing Masked().String().
🧹 Nitpick comments (1)
filter_test.go (1)
1269-1271: Add a mapped-IPv6 tunnel-key case here.This fixture only covers plain IPv4. Since the migrated encode path now branches on
netip.Addr.Is4()/Is6(), a::ffff:10.10.10.{1,2}subtest is the case that actually locks in the family-detection regression this PR is fixing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filter_test.go` around lines 1269 - 1271, Add a new subtest/fixture that uses an IPv4-mapped IPv6 address for the tunnel key to exercise the Is4/Is6 branching: create a TunnelKeyAction via NewTunnelKeyAction() and set tunnelAct.SrcAddr and tunnelAct.DstAddr to netip.MustParseAddr("::ffff:10.10.10.1") and netip.MustParseAddr("::ffff:10.10.10.2") (mirroring the existing IPv4 case), then include assertions identical to the IPv4 test so the encode path that checks netip.Addr.Is4()/Is6() is validated for mapped-IPv6 addresses as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@filter_test.go`:
- Around line 2158-2159: The test currently sets PeditAction.SrcIP but only
asserts the decoded type; update the test to assert the SrcIP value round-trips
correctly by comparing the original peditAction.SrcIP to the parsed/decoded
action's SrcIP (after extracting the decoded action from filter.Actions), so any
regression in SetSrcIP or parsePeditExtendedKeys fails the test; locate the
PeditAction instance and the decoded action assertion and add a strict equality
check between their SrcIP values.
In `@link.go`:
- Around line 1285-1289: Vti.Type() regresses when Local is the zero-value
because Is6() is false; change the check to mirror Gretap/Gretun by using
!vti.Local.Is4() (or equivalently the previous To4()==nil logic) so an
unset/IPv6 Local still returns "vti6"; update the conditional in Vti.Type to use
Local.Is4/Is6 consistently with Gretap and Gretun (reference: Vti.Type,
vti.Local, Local.Is4, Local.Is6, Gretap, Gretun).
In `@nl/tc_linux.go`:
- Around line 1551-1565: The SetDstIP and SetSrcIP methods on TcPedit must first
validate the netip.Addr to avoid routing invalid zero-value addresses into the
IPv6 path; add an ip.IsValid() guard at the start of both TcPedit.SetDstIP and
TcPedit.SetSrcIP and return early if false so you only call
SetIPv4Dst/SetIPv6Dst and SetIPv4Src/SetIPv6Src for valid addresses (this
prevents downstream calls like As16() on an invalid addr).
---
Outside diff comments:
In `@nl/tc_linux.go`:
- Around line 1626-1656: The IPv4 setters must validate and extract IPv4 bytes
explicitly: in TcPedit.SetIPv4Src and TcPedit.SetIPv4Dst check ip.Is4() and
return early if false, then call ip.As4() and pass that 4-byte slice to
NativeEndian().Uint32(...) (instead of ip.AsSlice()), keeping the rest of the
logic (tKey.Val, tKey.Off, tKeyEx.HeaderType/Cmd, appending to p.Keys/p.KeysEx
and incrementing p.Sel.NKeys) unchanged so they mirror the pattern used by
SetSrcIP/SetDstIP and the IPv6 setters.
---
Duplicate comments:
In `@rule_test.go`:
- Around line 676-679: The test currently compares prefixes using
Masked().String(), which misses the kernel's "from/to all" invalid vs explicit
/0 edge case; replace those comparisons with calls to the production helper
ruleIPNetEqual to ensure canonicalization matches RuleListFiltered
behavior—i.e., in the helper that checks a.Src vs b.Src and a.Dst vs b.Dst, call
ruleIPNetEqual(a.Src, b.Src) and ruleIPNetEqual(a.Dst, b.Dst) instead of
comparing Masked().String().
---
Nitpick comments:
In `@filter_test.go`:
- Around line 1269-1271: Add a new subtest/fixture that uses an IPv4-mapped IPv6
address for the tunnel key to exercise the Is4/Is6 branching: create a
TunnelKeyAction via NewTunnelKeyAction() and set tunnelAct.SrcAddr and
tunnelAct.DstAddr to netip.MustParseAddr("::ffff:10.10.10.1") and
netip.MustParseAddr("::ffff:10.10.10.2") (mirroring the existing IPv4 case),
then include assertions identical to the IPv4 test so the encode path that
checks netip.Addr.Is4()/Is6() is validated for mapped-IPv6 addresses as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 602bea86-b66c-467c-8e6b-07e79a3fb518
📒 Files selected for processing (26)
addr.goaddr_test.gobridge_linux_test.goconntrack_linux.goconntrack_test.gofilter_linux.gofilter_test.gogtp_linux.golink.golink_linux.gonetlink.gonexthop.gonexthop_linux.gonexthop_test.gonl/tc_linux.gonl/tc_linux_test.gonl/xfrm_linux.goroute_linux.goroute_test.gorule_linux.gorule_test.gosocket_linux.gosocket_test.goxfrm_policy_linux.goxfrm_policy_linux_test.goxfrm_state_linux.go
✅ Files skipped from review due to trivial changes (4)
- bridge_linux_test.go
- nexthop_test.go
- xfrm_policy_linux.go
- route_linux.go
🚧 Files skipped from review as they are similar to previous changes (8)
- nexthop.go
- socket_test.go
- nl/tc_linux_test.go
- conntrack_linux.go
- gtp_linux.go
- link_linux.go
- route_test.go
- socket_linux.go
| peditAction.SrcIP = netip.MustParseAddr("ffff::fff2") | ||
| filter.Actions = append(filter.Actions, peditAction) |
There was a problem hiding this comment.
Assert the migrated PeditAction.SrcIP round-trip.
The test now sets SrcIP, but it still only checks that the decoded action is a *PeditAction. A regression in SetSrcIP or parsePeditExtendedKeys would still pass.
🔧 Suggested assertion
- _, ok = flower.Actions[0].(*PeditAction)
+ pedit, ok := flower.Actions[0].(*PeditAction)
if !ok {
t.Fatal("Unable to find pedit action")
}
+ if pedit.SrcIP != peditAction.SrcIP {
+ t.Fatal("Pedit SrcIP doesn't match")
+ }Also applies to: 2200-2203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@filter_test.go` around lines 2158 - 2159, The test currently sets
PeditAction.SrcIP but only asserts the decoded type; update the test to assert
the SrcIP value round-trips correctly by comparing the original
peditAction.SrcIP to the parsed/decoded action's SrcIP (after extracting the
decoded action from filter.Actions), so any regression in SetSrcIP or
parsePeditExtendedKeys fails the test; locate the PeditAction instance and the
decoded action assertion and add a strict equality check between their SrcIP
values.
| func (vti *Vti) Type() string { | ||
| if vti.Local.To4() == nil { | ||
| if vti.Local.Is6() { | ||
| return "vti6" | ||
| } | ||
| return "vti" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "link\.go$"Repository: vishvananda/netlink
Length of output: 92
🏁 Script executed:
find . -name "link.go" -type fRepository: vishvananda/netlink
Length of output: 72
🏁 Script executed:
ls -la | head -20Repository: vishvananda/netlink
Length of output: 1243
🏁 Script executed:
find . -type f -name "*.go" | head -20Repository: vishvananda/netlink
Length of output: 415
🏁 Script executed:
cat -n link.go | sed -n '1280,1300p'Repository: vishvananda/netlink
Length of output: 536
🏁 Script executed:
wc -l link.goRepository: vishvananda/netlink
Length of output: 75
🏁 Script executed:
git log --oneline link.go | head -20Repository: vishvananda/netlink
Length of output: 105
🏁 Script executed:
git show HEAD:link.go | grep -A 20 "func (vti \*Vti) Type()"Repository: vishvananda/netlink
Length of output: 417
🏁 Script executed:
grep -n "func (gretap \*Gretap) Type()" link.goRepository: vishvananda/netlink
Length of output: 105
🏁 Script executed:
grep -n "func (gretun \*Gretun) Type()" link.goRepository: vishvananda/netlink
Length of output: 105
🏁 Script executed:
cat -n link.go | sed -n '1200,1220p'Repository: vishvananda/netlink
Length of output: 600
🏁 Script executed:
cat -n link.go | sed -n '1230,1250p'Repository: vishvananda/netlink
Length of output: 1146
🏁 Script executed:
grep -n "func.*Type() string" link.goRepository: vishvananda/netlink
Length of output: 1433
🏁 Script executed:
cat -n link.go | sed -n '1160,1170p'Repository: vishvananda/netlink
Length of output: 314
🏁 Script executed:
cat -n link.go | sed -n '1315,1325p'Repository: vishvananda/netlink
Length of output: 286
🏁 Script executed:
git log --all --oneline | grep -i "netip\|migration" | head -10Repository: vishvananda/netlink
Length of output: 105
🏁 Script executed:
git show 3ee2da8:link.go | grep -A 10 "func (vti \*Vti) Type()"Repository: vishvananda/netlink
Length of output: 222
🏁 Script executed:
git show 3ee2da8:link.go | grep -A 10 "func (gretap \*Gretap) Type()"Repository: vishvananda/netlink
Length of output: 237
🏁 Script executed:
git show 3ee2da8:link.go | grep -A 10 "func (gretun \*Gretun) Type()"Repository: vishvananda/netlink
Length of output: 209
🏁 Script executed:
git log --oneline link.go | head -5Repository: vishvananda/netlink
Length of output: 105
🏁 Script executed:
git show 3ee2da8^:link.go | grep -A 10 "func (vti \*Vti) Type()"Repository: vishvananda/netlink
Length of output: 101
🏁 Script executed:
git show 3ee2da8^:link.go | grep -A 10 "func (gretap \*Gretap) Type()"Repository: vishvananda/netlink
Length of output: 101
🏁 Script executed:
git show 3ee2da8^:link.go | grep -A 10 "func (gretun \*Gretun) Type()"Repository: vishvananda/netlink
Length of output: 101
🏁 Script executed:
git log --all --oneline | head -20Repository: vishvananda/netlink
Length of output: 1247
🏁 Script executed:
git show HEAD~1:link.go 2>/dev/null | grep -A 10 "func (vti \*Vti) Type()" || echo "No previous commits"Repository: vishvananda/netlink
Length of output: 82
🏁 Script executed:
git rev-list --all | head -10Repository: vishvananda/netlink
Length of output: 472
🏁 Script executed:
git log --all --graph --oneline | head -30Repository: vishvananda/netlink
Length of output: 1867
🏁 Script executed:
git branch -aRepository: vishvananda/netlink
Length of output: 160
🏁 Script executed:
git log --all --oneline -20Repository: vishvananda/netlink
Length of output: 1247
🏁 Script executed:
git log -p --all -- link.go | grep -B 5 -A 10 "func (vti \*Vti) Type()" | head -50Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
grep -B 20 "func (vti \*Vti) Type()" link.go | grep -A 20 "type Vti struct"Repository: vishvananda/netlink
Length of output: 272
🏁 Script executed:
cat -n link.go | sed -n '1265,1290p'Repository: vishvananda/netlink
Length of output: 595
🏁 Script executed:
cat > /tmp/test_netip.go << 'EOF'
package main
import (
"fmt"
"net/netip"
)
func main() {
var zeroAddr netip.Addr
fmt.Printf("Zero netip.Addr.Is4(): %v\n", zeroAddr.Is4())
fmt.Printf("Zero netip.Addr.Is6(): %v\n", zeroAddr.Is6())
fmt.Printf("Zero netip.Addr: %v\n", zeroAddr)
// Check behavior similar to old net.IP
// Old: net.IP(nil).To4() == nil was true for both nil and IPv6
// Let's see netip.Addr equivalence
var nilIP net.IP
fmt.Printf("nil net.IP.To4() == nil: %v\n", nilIP.To4() == nil)
fmt.Printf("nil net.IP.To6() == nil: %v\n", nilIP.To6() == nil)
}
EOF
go run /tmp/test_netip.go 2>/dev/null || echo "Go not available or compilation failed"Repository: vishvananda/netlink
Length of output: 101
🏁 Script executed:
grep -r "Vti.*Type()" . --include="*.go"Repository: vishvananda/netlink
Length of output: 104
🏁 Script executed:
grep -r "Type().*vti\|vti.*Type()" . --include="*.go" -iRepository: vishvananda/netlink
Length of output: 104
🏁 Script executed:
find . -name "*test*.go" -type f | grep -i vtiRepository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
find . -name "*test*.go" -type f | head -10Repository: vishvananda/netlink
Length of output: 238
🏁 Script executed:
grep -l "Vti" *.goRepository: vishvananda/netlink
Length of output: 97
🏁 Script executed:
grep -n "Vti\|vti" link_test.goRepository: vishvananda/netlink
Length of output: 368
🏁 Script executed:
git show 3ee2da8 -- link.go | head -200Repository: vishvananda/netlink
Length of output: 5368
🏁 Script executed:
git log -p --all -- link.go | grep -B 15 -A 5 "To4()" | head -100Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
git show c822ed7:link.go 2>/dev/null | grep -A 10 "func (vti \*Vti) Type()" || echo "File not found in c822ed7"Repository: vishvananda/netlink
Length of output: 229
🏁 Script executed:
git log --all -- link.go | tail -5Repository: vishvananda/netlink
Length of output: 239
🏁 Script executed:
git show ddba687:link.go 2>/dev/null | grep -A 10 "func (vti \*Vti) Type()" || echo "Not found"Repository: vishvananda/netlink
Length of output: 229
🏁 Script executed:
git show 3ee2da8 | head -300Repository: vishvananda/netlink
Length of output: 13356
🏁 Script executed:
git show 3ee2da8 --statRepository: vishvananda/netlink
Length of output: 6685
🏁 Script executed:
git show 3ee2da8 -- link.go | grep -A 30 "type Vti struct"Repository: vishvananda/netlink
Length of output: 570
🏁 Script executed:
git show 3ee2da8 -- link.go | grep -B 5 -A 15 "vti.Local"Repository: vishvananda/netlink
Length of output: 427
🏁 Script executed:
git diff 3ee2da8~1..3ee2da8 -- link.go 2>/dev/null | grep -A 10 -B 10 "Vti\|vti" || git log -1 --pretty=format:"%B" 3ee2da8Repository: vishvananda/netlink
Length of output: 97
Vti.Type() regresses when Local is unset.
Line 1286 switched from the old To4()==nil behavior to Local.Is6(). With netip.Addr, Is6() is false for the zero value, so a Vti with an unset Local now reports "vti" instead of "vti6". That changes link-kind selection for callers that only populate Remote or relied on the previous zero-value fallback. This inconsistency also differs from the pattern used in Gretap and Gretun, which both use !Local.Is4().
Suggested fix
func (vti *Vti) Type() string {
- if vti.Local.Is6() {
+ if !vti.Local.Is4() {
return "vti6"
}
return "vti"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (vti *Vti) Type() string { | |
| if vti.Local.To4() == nil { | |
| if vti.Local.Is6() { | |
| return "vti6" | |
| } | |
| return "vti" | |
| func (vti *Vti) Type() string { | |
| if !vti.Local.Is4() { | |
| return "vti6" | |
| } | |
| return "vti" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@link.go` around lines 1285 - 1289, Vti.Type() regresses when Local is the
zero-value because Is6() is false; change the check to mirror Gretap/Gretun by
using !vti.Local.Is4() (or equivalently the previous To4()==nil logic) so an
unset/IPv6 Local still returns "vti6"; update the conditional in Vti.Type to use
Local.Is4/Is6 consistently with Gretap and Gretun (reference: Vti.Type,
vti.Local, Local.Is4, Local.Is6, Gretap, Gretun).
| func (p *TcPedit) SetDstIP(ip netip.Addr) { | ||
| if ip.Is4() { | ||
| p.SetIPv4Dst(ip) | ||
| } else { | ||
| p.SetIPv6Dst(ip) | ||
| } | ||
| } | ||
|
|
||
| func (p *TcPedit) SetSrcIP(ip net.IP) { | ||
| if ip.To4() != nil { | ||
| func (p *TcPedit) SetSrcIP(ip netip.Addr) { | ||
| if ip.Is4() { | ||
| p.SetIPv4Src(ip) | ||
| } else { | ||
| p.SetIPv6Src(ip) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's net/netip package, does the zero-value Addr{}reportIs4() == false, and is Addr.IsValid() the intended way to reject that invalid value before dispatching on IP family?
💡 Result:
Yes, the zero-value net/netip.Addr{} reports Is4 == false. Yes, Addr.IsValid is the intended way to reject the invalid zero value before dispatching on IP family. The official documentation states: "The zero Addr is not a valid IP address." and "IsValid reports whether the Addr is an initialized address (not the zero Addr). Note that '0.0.0.0' and '::' are both valid values." The source code confirms the zero value has z == z0 (nil/invalid), while Is4 returns ip.z == z4, so false for zero. Methods like Is4In6 and IsPrivate explicitly return false for the zero Addr. As4 panics on zero Addr, emphasizing it must be checked first with IsValid before family-specific operations.
Citations:
- 1: https://pkg.go.dev/net/netip@master
- 2: https://go.dev/src/net/netip/netip.go?s=3029%3A3056
- 3: https://pkg.go.dev/net/netip
- 4: https://go.dev/src/net/netip/netip.go
- 5: https://tip.golang.org/src/net/netip/netip.go
- 6: https://go.pact.im/doc/src/net/netip/netip.go.html
- 7: https://pkg.go.dev/net/netip@go1.25.6
- 8: https://pkg.go.dev/net/netip@go1.26.0
- 9: https://ref.gotd.dev/pkg/net/netip.html
🏁 Script executed:
# First, locate the file and check its size
fd -n tc_linux.go | head -5Repository: vishvananda/netlink
Length of output: 293
🏁 Script executed:
# Get the actual implementation around lines 1551-1565 and surrounding setters
fd -n tc_linux.go -x wc -l {} \;Repository: vishvananda/netlink
Length of output: 293
🏁 Script executed:
# Read the relevant sections of nl/tc_linux.go
fd -n tc_linux.go -x cat -n {} \; | sed -n '1540,1700p'Repository: vishvananda/netlink
Length of output: 293
🏁 Script executed:
# Find tc_linux.go file
find . -name "tc_linux.go" -type fRepository: vishvananda/netlink
Length of output: 79
🏁 Script executed:
# Read the relevant sections once we locate the file
cat -n nl/tc_linux.go | sed -n '1540,1700p'Repository: vishvananda/netlink
Length of output: 4656
Add IsValid() guard before family dispatch in generic IP setters.
Lines 1551-1565: SetDstIP and SetSrcIP only test Is4() before dispatching, so an invalid zero-value netip.Addr (where Is4() == false) will route to the IPv6 path and call SetIPv6Dst, which immediately invokes As16() on an invalid address. After migration to the netip.Addr value type, invalid addresses are easier to pass accidentally and must be rejected explicitly.
Suggested guard
func (p *TcPedit) SetDstIP(ip netip.Addr) {
+ if !ip.IsValid() {
+ return
+ }
if ip.Is4() {
p.SetIPv4Dst(ip)
} else {
p.SetIPv6Dst(ip)
}
}
func (p *TcPedit) SetSrcIP(ip netip.Addr) {
+ if !ip.IsValid() {
+ return
+ }
if ip.Is4() {
p.SetIPv4Src(ip)
} else {
p.SetIPv6Src(ip)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nl/tc_linux.go` around lines 1551 - 1565, The SetDstIP and SetSrcIP methods
on TcPedit must first validate the netip.Addr to avoid routing invalid
zero-value addresses into the IPv6 path; add an ip.IsValid() guard at the start
of both TcPedit.SetDstIP and TcPedit.SetSrcIP and return early if false so you
only call SetIPv4Dst/SetIPv6Dst and SetIPv4Src/SetIPv6Src for valid addresses
(this prevents downstream calls like As16() on an invalid addr).
|
any opinion on this @aojea, @aboch or @vishvananda ? |
|
IMHO the amount of code changes required should be justified with a clear benchmark that shows the improvement and why is worth taking the risk |
|
My initial motivation was more to fix #1114 . There's some background here on the motivations behind this package: netip's predecessor Regarding the performance improvement itself, I didn't do any benchmark but it is supposed to be more memory efficient as stated in the go1.18 release notes |
|
not a maintainer here, I'm well aware of this work and I myself started this in Kubernetes some time ago, although we found some edges on things that were not working the same , that is like with my maintainer hat I will prefer to be conservative and just change only the parts that I know that I will get a benefit rather than explore a full entire migration |
|
my bad, I thought you were a maintainer! your feedback is valuable though, I agree with you that it would be easier to only fix the part where there is a real benefit. In my case that's only adding/removing routes with an IPv4-mapped IPv6 nexthop. |
|
pushed #1198 as an alternative, hope it will be more acceptable for the maintainers! |
2 participants
Simply picked up the original work from #1148 and rebased on top of master with some adjustments. @ShimmerGlass original commit authorship is preserved in git history.
Fixes #1114
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests