fix(cubenet): resolve direct on-link neighbors via ARP - #1321
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
AI-generated review (not human-approved)Overall. This is a well-structured fix for #1209. In direct egress mode the datapath previously sent everything to the node-gateway MAC, which breaks on-link traffic on networks without hairpin forwarding. The PR adds a The main concerns are ordering and a few correctness edge cases, listed below. None appear to be release-blocking, but several deserve a look before merge. Findings1. ARP resolution now happens before the NAT/policy checks (medium).
Worth confirming this is acceptable for the policy model. If not, the neighbor lookup could be deferred until after the session is created (accepting that the L2 rewrite then has to be re-done post-NAT). 2. 3. ARP-reply learning doesn't verify the reply's target IP (low/medium). 4. Subnet-directed broadcast addresses are classified as on-link (low). 5. Unrelated change bundled in (low). The removal of the two 6. ARP-conversion edge cases on GSO / CHECKSUM_PARTIAL skbs (low). Positive notes. The |
The CubeVS unit tests pass, and the BPF programs were also validated successfully on Linux 5.4.0-216-generic. |
| if _, err := ipMaskToUint32(net.CIDRMask(64, 128)); err == nil { | ||
| t.Fatal("ipMaskToUint32 accepted an IPv6 mask") | ||
| } | ||
| if _, err := ipMaskToUint32(net.IPMask{255, 0, 255, 0}); err == nil { |
There was a problem hiding this comment.
Failing test. ipMaskToUint32 does not reject this mask. net.IPMask{255, 0, 255, 0} has length 4, so the len(mask) != net.IPv4len guard passes, and net.IPMask.Size() returns (8, 32) for it — the bits != 32 guard passes for any 4-byte mask, contiguous or not. The function returns 0x00ff00ff, nil, so err == nil here and this t.Fatal fires, making TestIPMaskToUint32RejectsNonIPv4Mask fail. Either add a real contiguity check to ipMaskToUint32 (leading 0xff bytes followed by 0x00 bytes), or drop this assertion. As written, go test ./... in CubeNet/cubevs fails, which conflicts with the PR's validation claim.
| Mask: host.IPMask, | ||
| } | ||
| if err := ensureCubeDevIptablesRule( | ||
| "-t", "filter", "-A", "FORWARD", |
There was a problem hiding this comment.
Rule placement + no cleanup. The FORWARD ACCEPT rule is appended with -A, i.e. at the end of the filter/FORWARD chain. On hosts where that chain has a default-DROP policy or an earlier terminating rule (Docker, kube-proxy, distro firewalls), the first packet of a new on-link flow is dropped before it reaches this ACCEPT, so direct on-link egress silently fails. Consider -I FORWARD 1 (insert at the top) or otherwise guarantee the rule precedes any drop. Additionally there is no inverse cleanup: these sysctls and this rule are never removed when the node switches to route-aware mode (direct_egress_is_onlink becomes inactive) or when the node IP/mask changes — a stale rule with the old IP is left behind while a new one is appended each time the config changes.
| if err := systemnet.CleanupCubeRouter(cubeSNATPortMin()); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := systemnet.ConfigureCubeDevHostRouting(cubeDev, device); err != nil { |
There was a problem hiding this comment.
Breaking API change + mode-switch asymmetry. (1) NodeIPMask becomes a hard requirement for cubevs.Init: rewriteConstants fails with invalid IPv4 mask length: 0 whenever it is nil and the mvmtap object (which Init always loads) contains from_cube. Params is an exported struct, so any out-of-repo caller that constructs cubevs.Params without populating the new field now fails at load time. (2) This branch is asymmetric with the route-aware path: when CubeRouterEnable is later set on a node that previously ran direct egress, CleanupCubeRouter removes the router rules, but nothing undoes ConfigureCubeDevHostRouting — cube-dev keeps accept_local=1, rp_filter=2, forwarding=1, and the FORWARD rule indefinitely. A CleanupCubeDevHostRouting counterpart would make the two modes symmetric.
|
Redirect traffic to cube-dev looks wierd. Have you tired |
I looked into |
|
How about:
|
I gave this a quick try. It works once the map is populated, but the packet that triggers the miss gets dropped before userspace finishes ARP. The first ping/UDP packet was lost, and TCP connected after one retry (~1s). Is that acceptable? |
|
Given we don't have MAC in place, I think the lost is acceptable. A second thought: we can simply turn the lost packet to a ARP packet. |
| if len(mask) != net.IPv4len { | ||
| return 0, fmt.Errorf("invalid IPv4 mask length: %d", len(mask)) | ||
| } | ||
| if _, bits := mask.Size(); bits != 32 { |
There was a problem hiding this comment.
ipMaskToUint32 only checks the mask length via mask.Size().bits, which is always 32 for any 4-byte mask — contiguous or not. net.IPMask{255, 0, 255, 0} yields Size() == (8, 32), so this returns (0x00ff00ff, nil) instead of an error. Consequently the new TestIPMaskToUint32RejectsNonIPv4Mask (util_test.go) fails on its "non-contiguous IPv4 mask" case, so go test ./... in CubeNet/cubevs fails as submitted. Either validate contiguity (e.g. compare mask against net.CIDRMask(ones, 32)) or drop/relax that assertion.
| if params.EgressRedirectFlags != 0 { | ||
| return nil | ||
| } | ||
| directNeighborResolverOnce.Do(func() { |
There was a problem hiding this comment.
The resolver is gated by a package-level sync.Once whose error is cached forever, and both readEvents and readARPReplies return permanently on the first non-ErrClosed/non-EINTR error. If CubeVS is ever re-initialized (Init re-run), a new BPF load unlinks and re-creates direct_neigh/direct_neigh_ev, but the old goroutines keep reading the orphaned map instances and the newly created perf-event array has no reader — on-link resolution silently stops with no log. Worth tying the resolver to the same lifecycle as the rest of CubeVS (tear down on re-init, restart on transient failure) or at least documenting that Init is single-use.
| * ARP header. A delayed NIC checksum write therefore lands in padding rather | ||
| * than corrupting ar_tip. change_tail also clears any GSO state. | ||
| */ | ||
| err = bpf_skb_change_tail(skb, DIRECT_ARP_PRESERVED_LEN, 0); |
There was a problem hiding this comment.
The existing tcp_reply_reset guards bpf_skb_change_tail with if (skb->gso_segs) return TC_ACT_SHOT; because the helper "may fail on GSO skbs or leave segmentation state inconsistent" (see around line 265 in the base file). This new ARP-conversion path calls bpf_skb_change_tail + bpf_skb_change_head unconditionally. The stated validation used isolated network namespaces (veth pairs), which do not exercise GSO or CHECKSUM_PARTIAL offload that a real VM TAP device produces; the CHECKSUM_PARTIAL padding scheme in the comment also relies on skb->csum_start tracking the preserved bytes across bpf_skb_change_head, which is subtle. Please confirm this path is safe for GSO/checksum-offloaded TAP traffic, or add the same skb->gso_segs guard.
e798eb3 to
f149ddb
Compare
| * value: destination MAC address; all zeroes means resolution is pending | ||
| */ | ||
| struct { | ||
| __uint(type, BPF_MAP_TYPE_LRU_HASH); |
There was a problem hiding this comment.
Direct-neighbor entries have no TTL and are only refreshed when a future ARP packet from the peer hits learn_direct_neighbor (BPF_EXIST). The kernel's own neighbor table revalidates stale entries (NUD_STALE → probe, gc_stale_time); this LRU only evicts under pressure, which won't happen on a typical /24 (≤254 on-link hosts vs 8192 entries). If a peer changes its MAC without sending a gratuitous ARP (silent reboot, DHCP lease change, VM recreate) and traffic is mostly one-directional, redirect_egress keeps using the stale MAC indefinitely. Consider storing a timestamp in the value and re-ARPing after an idle threshold (or periodically clearing entries).
| static __always_inline bool direct_egress_is_onlink(__u32 daddr) | ||
| { | ||
| return egress_redirect_flags == 0 && | ||
| (daddr & nodenic_netmask) == (nodenic_ip & nodenic_netmask); |
There was a problem hiding this comment.
The pure prefix match also classifies the subnet's network and directed-broadcast addresses as on-link (e.g. for a /24 node, 10.2.3.0 and 10.2.3.255). A sandbox sending a UDP directed broadcast will have its packet consumed by direct_egress_arp_request (it becomes an ARP request for the broadcast address, which no host answers) instead of being delivered to the subnet. Consider excluding the network and broadcast addresses from the on-link set (e.g. daddr != nodenic_ip & nodenic_netmask and daddr != (nodenic_ip & nodenic_netmask) | ~nodenic_netmask).
| * this path. Until an ARP reply fills it, each packet retries the request. | ||
| */ | ||
| if (!neighbor) | ||
| bpf_map_update_elem(&direct_neigh, &daddr, &pending, BPF_NOEXIST); |
There was a problem hiding this comment.
Every packet to an unresolved on-link neighbor is converted into a fresh ARP request and the original data packet is discarded (the pending entry provides no throttling/backoff). Two consequences worth addressing or documenting:
- Unlike the kernel (which queues the first packet in the neighbor table and delivers it after ARP resolution), the first UDP datagram to a cold on-link neighbor is lost — the sender must retransmit.
- A sandbox flooding a dead on-link IP emits one broadcast ARP per packet out the node NIC, with no rate limiting (kernel ARP probes are rate-limited via
neigh_interval).
Consider storing a timestamp in the pending entry and only re-ARPing after a minimum interval.
jay3cx
changed the title
| * ARP header. A delayed NIC checksum write therefore lands in padding rather | ||
| * than corrupting ar_tip. change_tail also clears any GSO state. | ||
| */ | ||
| err = bpf_skb_change_tail(skb, DIRECT_ARP_PRESERVED_LEN, 0); |
There was a problem hiding this comment.
This bpf_skb_change_tail(skb, 96, 0) only succeeds for packets ≤ ~110 bytes. The kernel guard in net/core/filter.c is if (new_len < len - skb_network_offset(skb)) return -EINVAL; — for a TC packet skb_network_offset is 14 (Ethernet), so shrinking any packet longer than 110 bytes to 96 returns -EINVAL and the function returns TC_ACT_SHOT without emitting the ARP request.
Because redirect_egress() has already created/refreshed the pending entry, every subsequent packet to that destination is either rate-limited (next_probe_at_ns > now) or hits the same failed conversion — so an on-link destination whose triggering packet is > 110 bytes (any UDP datagram with payload > 68 bytes, a large ICMP echo, etc.) stays unresolved indefinitely and the flow blackholes. The PR's validation only exercised 98-byte ICMP pings, which sit just under the threshold, so this wasn't caught.
Note the same file already treats bpf_skb_change_tail as unsafe on large/GSO skbs: tcp_reply_reset guards with if (skb->gso_segs) return TC_ACT_SHOT; (line ~269) before calling it. This path has no equivalent guard, and the comment's claim that "change_tail also clears any GSO state" doesn't hold for the shrink case (skb_trim does not clear gso_segs/gso_size) — with TAP offloads TUN_F_TSO4/6 enabled, GSO skbs can reach from_cube.
Consider building the ARP frame with bpf_skb_store_bytes at the front of the (already large enough) packet without shrinking, or explicitly bounding the trigger-packet size before the conversion.
| neighbor_mac = (union macaddr *)neighbor->addr; | ||
| set_mac_pair(l2, nodenic_macaddr_p1, nodenic_macaddr_p2, | ||
| neighbor_mac->p1, neighbor_mac->p2); | ||
| return bpf_redirect(dst_ifindex, 0); |
There was a problem hiding this comment.
Learned entries are never re-validated. next_probe_at_ns is only maintained on the pending (zero-MAC) path, and learn_direct_neighbor writes learned entries with next_probe_at_ns = 0. Once a MAC is cached, this resolved branch is taken forever — there is no TTL and no re-ARP. If an on-link peer changes its MAC without a gratuitous ARP (NIC replacement, VM live-migration on the same L2, IP reassignment), traffic to it blackholes until LRU eviction (only under map pressure) or CubeVS restart.
The kernel's own neighbor cache re-resolves after NUD_STALE; this cache has no equivalent. Suggest using next_probe_at_ns to schedule periodic re-probing of resolved entries (e.g. after a few minutes of inactivity), or at least documenting the staleness trade-off.
| struct ethhdr *l2, __u32 daddr) | ||
| { | ||
| struct direct_neighbor pending = {}; | ||
| struct direct_neighbor *neighbor; |
There was a problem hiding this comment.
Revalidation consumes a data packet every DIRECT_NEIGH_REVALIDATE_INTERVAL_NS per destination: once the deadline passes, the first packet to a learned neighbor is turned into an ARP probe and dropped. In the steady state this silently drops one UDP datagram per destination every 5 minutes (and, while the peer is unresponsive to ARP, repeats roughly once per second). TCP tolerates this via retransmission, but on-link UDP flows will see periodic loss that the PR description does not mention. If the goal is only to revalidate a possibly-changed MAC, consider forwarding the packet with the cached MAC while issuing the probe in the background.
| eth_src = (union macaddr *)packet->eth.h_source; | ||
| arp_src = (union macaddr *)packet->arp.ar_sha; | ||
| /* Only trust a nonzero unicast sender MAC that matches the Ethernet header. */ | ||
| if (eth_src->p1 != arp_src->p1 || eth_src->p2 != arp_src->p2 || |
There was a problem hiding this comment.
learn_direct_neighbor accepts any valid unicast ARP reply whose ar_sip matches an existing direct_neigh key, with no check that the reply was solicited — e.g., ar_tip == nodenic_ip. Any host on the L2 segment can therefore populate the cache for a pending IP with its own MAC, and since this program runs on node NIC ingress it sees all LAN ARP traffic. Given the trusted-LAN model this may be acceptable, but adding packet->arp.ar_tip == nodenic_ip would make the learn strictly reply-driven and close the unsolicited/gratuitous-reply window.
|
|
||
| // ipMaskToUint32 converts an IPv4 mask to the same byte layout as ipToUint32. | ||
| func ipMaskToUint32(mask net.IPMask) uint32 { | ||
| return uint32(mask[0]) | uint32(mask[1])<<8 | uint32(mask[2])<<16 | uint32(mask[3])<<24 |
There was a problem hiding this comment.
ipMaskToUint32 indexes mask[0..3] with no length check, unlike ipToUint32 which handles 16-byte net.IP. Params.NodeIPMask is a net.IPMask; a nil or non-4-byte mask (an external caller of the cubevs package, or a device without an IPv4 mask) will panic inside Init() because rewriteConstants calls this unconditionally. The single in-repo caller always supplies a 4-byte IPv4 mask, but a guard (or an explicit length contract) would make this robust.
|
Thanks. Will take another round of review tomorrow. |
| } | ||
|
|
||
| now = bpf_ktime_get_ns(); | ||
| neighbor = bpf_map_lookup_elem(&direct_neigh, &daddr); |
There was a problem hiding this comment.
Before doing a map lookup, can we try bpf_fib_lookup first?
There was a problem hiding this comment.
fib_lookup first, then ARP if it misses.
| __u16 snat_id; | ||
| __u64 flags; | ||
| __u64 now; | ||
| int mac_result; |
There was a problem hiding this comment.
Sort variables in reverse xmas tree order.
| packet.arp.ar_sip = nodenic_ip; | ||
| packet.arp.ar_tip = daddr; | ||
|
|
||
| /* Leave padding after the ARP header so a delayed CHECKSUM_PARTIAL |
There was a problem hiding this comment.
It's hard to understand what's going on here. Need more comments.
d9d6953 to
2ec756e
Compare
| @@ -228,10 +229,6 @@ func attachTCFilter(progName string, ifindex uint32, direction TCDirection) erro | |||
|
|
|||
There was a problem hiding this comment.
The PR description says "Clear the direct-neighbor map when CubeVS starts to avoid reusing stale entries", but this diff doesn't do that — and it removes the only two pin-cleanups that existed.
The new direct_neigh map in map.h is pinned with LIBBPF_PIN_BY_NAME, and loadObject opens a collection with Maps.PinPath set. cilium/ebpf reuses an already-pinned map instead of creating a fresh one, so its contents (MAC + next_probe_at_ns deadlines) survive a CubeVS restart. A stale entry whose next_probe_at_ns is still in the future will be used directly (goto set_neighbor in prepare_egress_l2), so a neighbor whose MAC changed while CubeVS was down blackholes traffic for up to DIRECT_NEIGH_REVALIDATE_INTERVAL_NS (5 min) — exactly the failure mode the PR says it prevents.
To clear the map on start, Init needs something like _ = os.Remove(pinPath(MapNameDirectNeigh)). As written, this is missing.
Separately, removing the os.Remove for tungrp_to_tuns and dns_query_track is unrelated to this feature and unexplained. dns_query_track cleanup existed on purpose ("runtime pending-query state, not persisted policy"); now stale pending-DNS entries survive restarts (low impact given the 10 s TTL, but the change isn't mentioned in the PR body).
| return EGRESS_MAC_DROP; | ||
| } | ||
|
|
||
| if (direct_egress_arp_request(skb, daddr)) |
There was a problem hiding this comment.
Revalidation consumes a live data packet even when the cached MAC is still valid.
When the entry has expired (next_probe_at_ns <= now) but addr is a valid, non-zero MAC, control falls through to direct_egress_arp_request() and the current data packet is converted into an ARP probe (EGRESS_MAC_PROBE → redirect). The packet's payload is discarded, so every DIRECT_NEIGH_REVALIDATE_INTERVAL_NS (5 min) per on-link destination, one data packet is silently dropped: TCP pays a retransmission, UDP loses the datagram.
This is also fragile for GSO/large packets: this codebase already notes in tcp_reply_reset() that bpf_skb_change_tail() "may fail on GSO skbs". If the first packet after expiry is a large GSO segment, direct_egress_arp_request fails, the packet is dropped, next_probe_at_ns was already set to now + 1 s, and a sustained large-packet flow keeps hitting the same failed probe every second until a small packet slips through — effectively stalling the flow for up to 5 minutes.
Consider rewriting the current packet with the cached MAC (forward it now) and setting the deadline so a later packet triggers the probe, rather than sacrificing the triggering packet.
|
|
||
| /* Touch only the deadline so a concurrent learn cannot lose its MAC. */ | ||
| retry_at = now + DIRECT_NEIGH_PROBE_INTERVAL_NS; | ||
| neighbor->next_probe_at_ns = retry_at; |
There was a problem hiding this comment.
Unlocked in-place write to the map value races with learn_direct_neighbor's whole-value update on another CPU.
neighbor->next_probe_at_ns = retry_at; writes directly into the map value through the lookup pointer (no per-entry lock), while learn_direct_neighbor concurrently overwrites the whole struct direct_neighbor via bpf_map_update_elem(..., BPF_EXIST) on another CPU. Two consequences:
-
Torn MAC exposure:
learn_direct_neighborstoresmac->p1andmac->p2as two separate stores. A concurrent reader reachingset_neighbor(lines 261-263) can observe p1 from the new MAC and p2 from the old one, and emit frames to a bogus MAC. Because the learned deadline (now + REVALIDATE_INTERVAL) stays in the future, that corrupted entry is used for up to 5 minutes until the next probe. -
Deadline clobber: if this write lands after a concurrent learn, the learned
next_probe_at_ns(now + 5 min) is overwritten back down tonow + 1 s, so the entry re-enters the probe path a second later and a spurious ARP probe consumes another packet.
The re-lookup after the write (intended to detect the concurrent learn) narrows but does not close this window. Consider making the MAC + deadline a single 64-bit-aligned store, or guarding the value with bpf_spin_lock.
| packet->arp.ar_pro != bpf_htons(ETH_P_IP) || | ||
| packet->arp.ar_hln != ETH_ALEN || | ||
| packet->arp.ar_pln != sizeof(__be32) || | ||
| packet->arp.ar_op != bpf_htons(ARPOP_REPLY)) |
There was a problem hiding this comment.
The "peer announces a MAC change" refresh in the comment above only works for ARP replies, not gratuitous ARP.
This check requires ar_op == ARPOP_REPLY, but a MAC change is normally announced with a gratuitous ARP request (ar_op == ARPOP_REQUEST, ar_tip = own IP). Such packets are silently ignored here, so the "refresh an existing entry when the peer announces a MAC change" behavior described in the comment (lines 20-22) is not actually implemented — the only refresh path is when the peer answers one of our probes. It still eventually catches up via DIRECT_NEIGH_REVALIDATE_INTERVAL_NS, so impact is limited, but the comment overstates the behavior.
Also, the validation accepts an unsolicited ARP reply from any sender whose IP currently has a pending/known entry, without checking that the reply targets our node (ar_tip == nodenic_ip / ar_tha == node MAC). Any on-link host can therefore refresh an existing entry's MAC on demand. That is equivalent to ordinary ARP spoofing on the same segment, but if stricter validation is desired, requiring ar_tip == nodenic_ip would drop unsolicited replies that are not responses to our probes.
2ec756e to
d2833e4
Compare
Use bpf_fib_lookup on the node NIC first. If the kernel already has a neighbor, reuse that MAC; otherwise keep the existing map and ARP path. Signed-off-by: jay3cx <137191587@qq.com>
d2833e4 to
8afbd05
Compare
| * change_tail(96) keeps that slot. change_head(32) moves it past ARP. | ||
| * Then store ARP and zero the rest (broadcast). | ||
| */ | ||
| err = bpf_skb_change_tail(skb, DIRECT_ARP_PRESERVED_LEN, 0); |
There was a problem hiding this comment.
bpf_skb_change_tail() on a GSO skb can leave segmentation state inconsistent. tcp_reply_reset() in this same file explicitly guards if (skb->gso_segs) return TC_ACT_SHOT; before its bpf_skb_change_tail() call. The cold-miss / revalidation path here can be triggered by a large guest TCP/UDP segment (virtio-net GSO delivers such packets as single skbs with gso_size/gso_segs set), which would be truncated to 96 bytes with GSO metadata still intact — risking a malformed ARP frame on transmit. Recommend dropping when skb->gso_segs != 0 (as tcp_reply_reset does) or clearing GSO state before the rewrite.
|
|
||
| /* Touch only the deadline so a concurrent learn cannot lose its MAC. */ | ||
| retry_at = now + DIRECT_NEIGH_PROBE_INTERVAL_NS; | ||
| neighbor->next_probe_at_ns = retry_at; |
There was a problem hiding this comment.
Revalidation consumes the triggering data packet: when a learned entry's next_probe_at_ns deadline passes, this packet is converted into an ARP request instead of being forwarded with the already-known MAC. With DIRECT_NEIGH_REVALIDATE_INTERVAL_NS = 5 min, every on-link destination silently drops one packet every 5 minutes; a persistently unreachable peer costs one packet per second. For TCP this surfaces as an RTO retransmission, but for UDP/ICMP it is silent loss. The PR documents this tradeoff, but it is a user-visible behavior change — consider forwarding the current packet with the known MAC and probing out-of-band (e.g., a lightweight standalone probe) instead.
| @@ -228,10 +229,6 @@ func attachTCFilter(progName string, ifindex uint32, direction TCDirection) erro | |||
|
|
|||
| // Init should be called once before invoking any other CubeVS APIs. | |||
There was a problem hiding this comment.
This removal is unrelated to the ARP feature and changes reload semantics. The deleted comment states dns_query_track is "runtime pending-query state, not persisted policy" — i.e., deliberately cleared on each Init. With the cleanup gone, stale DNS pending-query entries survive CubeVS restarts because the pinned map is reused from bpffs. If the goal was only to let the new direct_neigh cache survive reloads, these removals should be reverted or explicitly called out in the PR description.
| /* Primary IPv4 prefix of the node NIC. Direct mode only. */ | ||
| static __always_inline bool direct_egress_is_onlink(__u32 daddr) | ||
| { | ||
| return egress_redirect_flags == 0 && |
There was a problem hiding this comment.
direct_egress_is_onlink() also matches subnet-directed broadcasts (e.g., 10.0.0.255 for a /24 node prefix). In direct mode the first broadcast packet is rewritten into an ARP request for an address nobody answers, and subsequent broadcasts are dropped during the 1 s probe window — a regression from the previous gateway-MAC behavior. If any workload relies on directed broadcast in direct mode, consider excluding broadcast/multicast addresses from the on-link ARP path (e.g., reject when daddr is the subnet or network address).
|
@jay3cx I think we can place the MAC address preparation work right after: CubeSandbox/CubeNet/src/mvmtap.bpf.c Lines 1021 to 1033 in ca3adc9 So that we don't have to touch protocol-specific code. |
Move neighbor lookup and ARP conversion out of the per-protocol NAT helpers so from_cube rewrites L2 in one place before those helpers run. Signed-off-by: jay3cx <137191587@qq.com>
04d6e3d to
a5538f8
Compare
| return EGRESS_MAC_READY; | ||
| } | ||
|
|
||
| err = bpf_fib_lookup(skb, &fib, sizeof(fib), |
There was a problem hiding this comment.
The PR targets Linux 5.4, but BPF_FIB_LOOKUP_DIRECT was only introduced in a later kernel (5.10). On 5.4 the helper will almost certainly reject the unknown flag (-EINVAL), so this fast path silently never fires and every on-link packet falls through to the neighbor-map/ARP path. The fallback still works (and was validated on 5.4), so this is not a functional break — but worth verifying: if 5.4 doesn't support the flag, either gate it on the kernel version or drop the direct flag on old kernels, and note that BPF_FIB_LOOKUP_OUTPUT can also trigger the kernel's own neighbor resolution (duplicate ARP requests on the segment).
| return bpf_redirect(cubegw0_ifindex, BPF_F_INGRESS); | ||
| } | ||
|
|
||
| mac_result = prepare_egress_l2(skb, l2, daddr); |
There was a problem hiding this comment.
Calling prepare_egress_l2() here moves ARP resolution before the NAT/policy checks that live in do_tcp_nat/do_udp_nat_inline/do_icmp_nat (policy deny + RST-on-deny happen inside create_nat_session). Consequences:
- A policy-denied on-link destination now still emits an ARP request on the wire (the base code never touched the wire for denied flows).
- The first packet of every flow to a new/expired on-link neighbor is consumed as an ARP probe — including the SYN that would previously have drawn an immediate RST-on-deny; the RST only appears ~1s later once the neighbor is learned.
Confirm this ordering is acceptable for the policy model; if not, the neighbor lookup would need to run after session creation.
| packet->arp.ar_pro != bpf_htons(ETH_P_IP) || | ||
| packet->arp.ar_hln != ETH_ALEN || | ||
| packet->arp.ar_pln != sizeof(__be32) || | ||
| packet->arp.ar_op != bpf_htons(ARPOP_REPLY)) |
There was a problem hiding this comment.
This accepts any ARP reply whose sender MAC matches the Ethernet source, without checking that the reply's target IP (ar_tip) is the node's own IP (nodenic_ip). The BPF_EXIST guard limits learning to IPs already probed, but within that set a spoofed/unsolicited self-consistent reply (e.g. from an on-link attacker who observed the probe) populates the cache. Consider also requiring packet->arp.ar_tip == nodenic_ip so only replies addressed to the node are learned.
| @@ -228,10 +229,6 @@ func attachTCFilter(progName string, ifindex uint32, direction TCDirection) erro | |||
|
|
|||
| // Init should be called once before invoking any other CubeVS APIs. | |||
| func Init(params Params) error { | |||
There was a problem hiding this comment.
This cleanup removal looks unrelated to the on-link-neighbor feature and changes restart semantics: a stale pinned dns_query_track map is now reused across CubeVS restarts instead of being cleared (the old comment explicitly said it's runtime pending-query state, not persisted policy), and a stale tungrp_to_tuns pin file lingers on bpffs. Probably benign (entries are TTL'd), but worth a comment explaining why, or splitting into a separate change.
| static __always_inline bool direct_egress_is_onlink(__u32 daddr) | ||
| { | ||
| return egress_redirect_flags == 0 && | ||
| (daddr & nodenic_netmask) == (nodenic_ip & nodenic_netmask); |
There was a problem hiding this comment.
This prefix match also classifies the subnet-directed broadcast address (e.g. x.x.x.255 on a /24) as on-link. If bpf_fib_lookup doesn't resolve it to the all-ones MAC, the packet gets converted into an ARP request for the broadcast address (which no host answers) and consumed, changing existing broadcast semantics. Consider excluding broadcast/multicast destinations (INADDR_BROADCAST, ipv4_is_multicast) from the on-link/ARP path.
@chenhengqi Moved it there. |
| neighbor = bpf_map_lookup_elem(&direct_neigh, &daddr); | ||
| if (!neighbor) | ||
| return EGRESS_MAC_DROP; | ||
| /* Later deadline: another CPU already refreshed this entry. */ | ||
| if (!direct_neighbor_is_zero(neighbor) && | ||
| neighbor->next_probe_at_ns > retry_at) | ||
| goto set_neighbor; |
There was a problem hiding this comment.
We can simply always do an ARP probe here, right? I am not sure how frequent this concurrent learning will happen.
| 2 * DIRECT_ARP_ZERO_CHUNK_LEN, 0); | ||
| } | ||
|
|
||
| static __always_inline long direct_egress_arp_request(struct __sk_buff *skb, __u32 daddr) |
There was a problem hiding this comment.
See
CubeSandbox/CubeNet/src/mvmtap.bpf.c
Line 253 in 189d6ee
I still have doubt in the implementation here. Please explain the purpose of bpf_skb_change_tail/bpf_skb_change_head/direct_egress_clear_arp_padding here. I haven't consult an AI agent yet.
2 participants
Motivation
In direct egress mode, CubeVS sends outbound traffic to the node gateway MAC.
This works for off-link destinations, but on-link traffic then depends on the gateway forwarding the packet back through the same interface. On networks without that hairpin behavior, same-subnet destinations are unreachable.
What Changed
bpf_fib_lookupfirst. If the kernel has no neighbor, replace the packet with an ARP request and keep a pending map entry.from_worlddispatch path inlined so Linux 5.4 can load the DNS tail call.A cache miss consumes the triggering packet (it becomes the ARP request). A later revalidation probe does the same. No userspace resolver or host iptables/sysctl setup is required.
Validation
go test -count=1 ./...andgo vet ./...inCubeNet/cubevs.go test -count=1 ./network/runtime/systemnet ./network/runtimeandgo vet ./network/runtime/...inCubelet.5.4.0-216-genericon arm64:from_cubeandfrom_worldand confirmed that both were JIT-compiled.Scope
This change only affects IPv4 on-link traffic in direct egress mode.
Off-link direct egress and route-aware egress keep their existing behavior.
Fixes #1209
Signed-off-by: jay3cx 137191587@qq.com