Skip to content

fix(cubenet): resolve direct on-link neighbors via ARP #1321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jay3cx wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix(cubenet): resolve direct on-link neighbors via ARP #1321

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b88b328
fix(cubenet): resolve direct on-link neighbors via ARP
jay3cx Aug 12, 2026
f5267bb
fix(cubenet): simplify userspace and refresh learned neighbors
jay3cx Aug 13, 2026
8afbd05
fix(cubenet): try fib lookup before the neighbor cache
jay3cx Aug 14, 2026
a5538f8
fix(cubenet): prepare on-link L2 once in from_cube
jay3cx Aug 17, 2026
0d1bccf
Merge remote-tracking branch 'upstream/master' into fix/issue-1209-di…
Aug 19, 2026
1699a2d
fix(cubenet): clarify CHECKSUM_PARTIAL ARP comment
jay3cx Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CubeNet/cubevs/cubevs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type Params struct {
// Ifindex, IP and MAC address of Node itself
NodeIfindex uint32
NodeIP net.IP
NodeIPMask net.IPMask
NodeMacAddr net.HardwareAddr
// MAC address of the Node gateway (next hop)
NodeGatewayMacAddr net.HardwareAddr
Expand Down Expand Up @@ -174,6 +175,7 @@ const (
globalNameEgressDMacaddrP2 = "egress_dmacaddr_p2"
globalNameEgressRedirectFlags = "egress_redirect_flags"
globalNameNodeIP = "nodenic_ip"
globalNameNodeNetmask = "nodenic_netmask"
globalNameNodeIfindex = "nodenic_ifindex"
globalNameNodeMacaddrP1 = "nodenic_macaddr_p1"
globalNameNodeMacaddrP2 = "nodenic_macaddr_p2"
Expand Down
5 changes: 1 addition & 4 deletions CubeNet/cubevs/miscs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func rewriteConstants(vars map[string]*ebpf.VariableSpec, params Params) error {
err = errors.Join(err, v.Set(params.EgressRedirectFlags))
}
err = errors.Join(err, vars[globalNameNodeIP].Set(ipToUint32(params.NodeIP)))

@cubesandboxbot cubesandboxbot Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipMaskToUint32(nil) returns "invalid IPv4 mask length: 0", so any CubeVS consumer that constructs Params without the new NodeIPMask field now hard-fails at loadMvmtap — a backward-incompatibility for the new field. The new test only covers the localgw (no from_cube) path with a nil mask, so the mvmtap nil case is untested. Consider treating a nil mask as "skip, leave the BPF default", and nil-check vars[globalNameNodeNetmask] before .Set so the dereference can't panic if the constant is ever absent from an object.

err = errors.Join(err, vars[globalNameNodeNetmask].Set(ipMaskToUint32(params.NodeIPMask)))
err = errors.Join(err, vars[globalNameNodeIfindex].Set(params.NodeIfindex))
err = errors.Join(err, vars[globalNameNodeMacaddrP1].Set(hardwareAddrToUint32(params.NodeMacAddr)))
err = errors.Join(err, vars[globalNameNodeMacaddrP2].Set(hardwareAddrToUint16(params.NodeMacAddr)))
Expand Down Expand Up @@ -228,10 +229,6 @@ func attachTCFilter(progName string, ifindex uint32, direction TCDirection) erro

@cubesandboxbot cubesandboxbot Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// Init should be called once before invoking any other CubeVS APIs.
Comment thread
jay3cx marked this conversation as resolved.
func Init(params Params) error {
Comment thread
jay3cx marked this conversation as resolved.
Outdated
_ = os.Remove(pinPath("tungrp_to_tuns")) // NOCC:Path Traversal()
// dns_query_track is runtime pending-query state, not persisted policy.
_ = os.Remove(pinPath(MapNameDNSQueryTrack)) // NOCC:Path Traversal()

err := loadObject(params, loadLocalgw, "loadLocalgw")
if err != nil {
return err
Expand Down
5 changes: 5 additions & 0 deletions CubeNet/cubevs/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ func ipToUint32(ip net.IP) uint32 {
return uint32(ip[0]) | uint32(ip[1])<<8 | uint32(ip[2])<<16 | uint32(ip[3])<<24
}

// 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

@cubesandboxbot cubesandboxbot Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread
jay3cx marked this conversation as resolved.
Comment thread
jay3cx marked this conversation as resolved.
}

// hardwareAddrToUint32 converts the first 4 bytes of MAC address to a uint32.
func hardwareAddrToUint32(addr net.HardwareAddr) uint32 {
return uint32(addr[0]) | uint32(addr[1])<<8 | uint32(addr[2])<<16 | uint32(addr[3])<<24
Expand Down
17 changes: 17 additions & 0 deletions CubeNet/src/cubevs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
/* ARP hardware types */
#define ARPHRD_ETHER 1 /* Ethernet */

/* https://elixir.bootlin.com/linux/v5.4.217/source/include/linux/socket.h#L172 */
#define AF_INET 2

#define MAX_ENTRIES 8192
#define MAX_IP_RULE_ENTRIES 8192
#define MAX_DOMAIN_RULE_ENTRIES 1024
Expand All @@ -38,6 +41,8 @@
#define NET_POLICY_FLAG_L7_REQUIRED 1
#define NSEC_PER_SEC 1000000000ULL
#define DNS_QUERY_TRACK_TTL_NS (10ULL * NSEC_PER_SEC)
#define DIRECT_NEIGH_PROBE_INTERVAL_NS (1ULL * NSEC_PER_SEC)
#define DIRECT_NEIGH_REVALIDATE_INTERVAL_NS (5ULL * 60 * NSEC_PER_SEC)

/* https://en.wikipedia.org/wiki/IPv4#Header
*
Expand Down Expand Up @@ -99,6 +104,7 @@ const volatile __u64 egress_redirect_flags = BPF_F_INGRESS;

/* Ifindex, IP and MAC address of Node itself */
const volatile __u32 nodenic_ip = 0x020a8709; /* 9.135.10.2, network byte order */
const volatile __u32 nodenic_netmask = 0x00ffffff; /* 255.255.255.0, packet-byte layout */
const volatile __u32 nodenic_ifindex = 2;
const volatile __u32 nodenic_macaddr_p1 = 0x68005452; /* 52:54:00:68:dd:16 */
const volatile __u16 nodenic_macaddr_p2 = 0x16dd;
Expand Down Expand Up @@ -129,6 +135,17 @@ struct arphdr_eth {
__be32 ar_tip; /* target IP address */
} __attribute__((packed));

struct arp_packet {
struct ethhdr eth;
struct arphdr_eth arp;
} __attribute__((packed));

struct direct_neighbor {
unsigned char addr[ETH_ALEN];
__u16 reserved;
__u64 next_probe_at_ns;
};

union macaddr {
struct {
__u32 p1;
Expand Down
14 changes: 14 additions & 0 deletions CubeNet/src/map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,20 @@ struct {
__uint(pinning, LIBBPF_PIN_BY_NAME);
} snat_iplist SEC(".maps");

/* Direct-egress on-link neighbor cache.
*
* key: destination IPv4 address in packet-byte layout
* value: destination MAC plus probe deadline; an all-zero MAC means initial
* resolution is pending
*/
struct {
__uint(type, BPF_MAP_TYPE_LRU_HASH);
Comment thread
jay3cx marked this conversation as resolved.
__uint(max_entries, MAX_ENTRIES);
__type(key, __u32);
__type(value, struct direct_neighbor);

@cubesandboxbot cubesandboxbot Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale direct_neigh entries survive CubeVS restarts, and the claimed startup cleanup is missing. This map is pinned with LIBBPF_PIN_BY_NAME, so a Cubelet/CubeVS restart reuses the existing pin (cilium/ebpf does not clear it); a previously-learned MAC keeps being used until next_probe_at_ns expires (up to DIRECT_NEIGH_REVALIDATE_INTERVAL_NS = 5 min after the last ARP reply).

The PR description says "Clear the direct-neighbor map when CubeVS starts to avoid reusing stale entries", but no such code exists anywhere in the diff. Meanwhile, Init() in miscs.go deletes the pre-existing os.Remove(pinPath(MapNameDNSQueryTrack)) cleanup (plus the already-dead tungrp_to_tuns removal). Was the intent to add os.Remove(pinPath("direct_neigh")) in Init() and the wrong lines were removed? If the cache really should be cleared at startup, that line is missing; if not, deleting the dns_query_track cleanup is an unrelated behavioral change (stale pending DNS-query state can persist across restarts).

__uint(pinning, LIBBPF_PIN_BY_NAME);
} direct_neigh SEC(".maps");

/* Egress allow list v2 (hash of maps)
*
* key: ifindex of the TAP device
Expand Down
182 changes: 176 additions & 6 deletions CubeNet/src/mvmtap.bpf.c
View file
Open in desktop

@chenhengqi chenhengqi Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change set_mac_pair() instead of touching every redirect_* in this file?

@jay3cx jay3cx Aug 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it into the old set_mac_pair sites so we don't touch every redirect.

Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,164 @@ static __always_inline bool should_do_nat(const struct iphdr *l3)
return true;
}

/* Primary IPv4 prefix of the node NIC. Direct mode only. */
static __always_inline bool direct_egress_is_onlink(__u32 daddr)

@cubesandboxbot cubesandboxbot Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

direct_egress_is_onlink classifies a destination as on-link iff it falls inside the node's primary IPv4 prefix. When that prefix is broader than the actual L2 segment — e.g., a node at 10.0.0.2 with a /8 primary mask, or a multi-homed host with other directly-connected subnets — an off-link destination inside the prefix (say 10.5.5.5, reachable only via the default gateway) is treated as on-link: redirect_egress converts the packet into a broadcast ARP request, nobody answers, and the packet is consumed/dropped. Before this change such traffic was delivered via the gateway MAC. This is documented as a deliberate limitation in the PR, and the "no hairpin" bug it solves is real, but the flip side is a functional regression in exactly those larger-prefix topologies — worth confirming that's an accepted tradeoff for the deployment's netmask, and ideally falling back to the gateway path when the ARP stays unresolved for a bounded number of probes.

{
return egress_redirect_flags == 0 &&

@cubesandboxbot cubesandboxbot Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix-based on-link classification will also match the subnet broadcast address (e.g. 10.0.0.255/24), which is then ARP-resolved and never answered — the packet is dropped and a permanent pending entry is created. It also ARP-resolves any in-prefix IP that is only actually reachable via the gateway on non-flat networks (multi-homed hosts, routed subnets, proxy-ARP), a behavioral regression vs. the previous always-gateway path. This is a documented tradeoff for the hairpin fix, but consider special-casing broadcast/multicast (send to ff:ff:ff:ff:ff:ff / the gateway path) and validating that the deployment subnet is truly flat L2.

Comment thread
jay3cx marked this conversation as resolved.
(daddr & nodenic_netmask) == (nodenic_ip & nodenic_netmask);
Comment thread
jay3cx marked this conversation as resolved.
Comment thread
jay3cx marked this conversation as resolved.
}

static __always_inline bool direct_neighbor_is_zero(const struct direct_neighbor *neighbor)
{
const union macaddr *macaddr = (const union macaddr *)neighbor->addr;

return macaddr->p1 == 0 && macaddr->p2 == 0;
}

#define DIRECT_ARP_PRESERVED_LEN 96
#define DIRECT_ARP_HEADROOM 32
#define DIRECT_ARP_FRAME_LEN (DIRECT_ARP_PRESERVED_LEN + DIRECT_ARP_HEADROOM)
#define DIRECT_ARP_ZERO_CHUNK_LEN 32

static __always_inline long direct_egress_clear_arp_padding(struct __sk_buff *skb)
{
unsigned char zeroes[DIRECT_ARP_ZERO_CHUNK_LEN] = {};
long err;

err = bpf_skb_store_bytes(skb, sizeof(struct arp_packet),
zeroes, sizeof(zeroes), 0);
if (err)
return err;
err = bpf_skb_store_bytes(skb,
sizeof(struct arp_packet) + DIRECT_ARP_ZERO_CHUNK_LEN,
zeroes, sizeof(zeroes), 0);
if (err)
return err;
return bpf_skb_store_bytes(skb,
sizeof(struct arp_packet) + 2 * DIRECT_ARP_ZERO_CHUNK_LEN,
zeroes,
DIRECT_ARP_FRAME_LEN - sizeof(struct arp_packet) -
2 * DIRECT_ARP_ZERO_CHUNK_LEN, 0);
}

static __always_inline long direct_egress_arp_request(struct __sk_buff *skb, __u32 daddr)

@chenhengqi chenhengqi Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

CubeSandbox/CubeNet/src/mvmtap.bpf.c

Line 253 in 189d6ee

static __always_inline int tcp_reply_reset(struct __sk_buff *skb, __u32 ifindex)

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.

@chenhengqi chenhengqi Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jay3cx

@jay3cx jay3cx Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TAP packets can still have CHECKSUM_PARTIAL set. After we turn the packet into an ARP request, that state is still there: __bpf_skb_change_tail() trims through bpf_skb_trim_rcsum(), which does not clear ip_summed, so the original csum_start survives all the way to dev_queue_xmit(). skb_checksum_help() then writes the L4 checksum at the old offset, and it BUG_ONs if that write would land past skb_headlen(). Getting the length wrong panics the kernel. It does not just emit a bad ARP.

bpf_skb_change_tail() is there because the helper will not shrink below __bpf_skb_min_len(). For CHECKSUM_PARTIAL that floor is skb_checksum_start_offset() + csum_offset + sizeof(__sum16). On a normal TCP packet that is 52 bytes, so bpf_skb_change_tail(skb, 42, 0) returns -EINVAL. 96 is just a length above that floor. Anything ≥ 52 would load. We cannot shrink straight to a 42-byte ARP frame.

Staying above that floor only keeps the write in bounds. It does not keep it off the ARP header. That is what bpf_skb_change_head(32) is for. I checked this on 5.4.0-216-generic with tx-checksumming off so skb_checksum_help() actually runs. With only change_tail(96), offset 50 is eb92. With change_tail(96) plus change_head(32), offset 50 is zero and ffff shows up at offset 82, which is 50 + 32.

direct_egress_clear_arp_padding() then zeros the rest of the 128-byte buffer so we don't broadcast leftover payload. I dumped the frame. Nothing from the original packet is left in it.

tcp_reply_reset() does not need any of this. It still has valid IP and TCP headers, and the checksum field is where the stack expects it.

I'll fix the comment in the code too. "keeps that slot" is the wrong picture.

{
struct arp_packet packet = {};
union macaddr *macaddr;
long err;

@cubesandboxbot cubesandboxbot Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bpf_skb_change_tail() can fail on GSO skbs — the codebase itself guards skb->gso_segs before calling it in tcp_reply_reset() (mvmtap.bpf.c:269, "may fail on GSO skbs or leave segmentation state inconsistent"). Here a failure returns TC_ACT_SHOT, but the pending direct_neigh entry was already inserted with a future deadline and a zero MAC, so every subsequent packet to that destination is dropped for DIRECT_NEIGH_PROBE_INTERVAL_NS and then another failing change_tail is attempted. A large GSO/UFO UDP datagram to a cold on-link destination could therefore be blackholed indefinitely instead of ever being resolved. Consider guarding skb->gso_segs (drop early so the next retry can resolve the neighbor) or falling back to the gateway-MAC path for un-resolvable GSO packets.

__builtin_memset(packet.eth.h_dest, 0xff, ETH_ALEN);
macaddr = (union macaddr *)packet.eth.h_source;
macaddr->p1 = nodenic_macaddr_p1;
macaddr->p2 = nodenic_macaddr_p2;
packet.eth.h_proto = bpf_htons(ETH_P_ARP);

packet.arp.ar_hrd = bpf_htons(ARPHRD_ETHER);
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_REQUEST);
macaddr = (union macaddr *)packet.arp.ar_sha;
macaddr->p1 = nodenic_macaddr_p1;
macaddr->p2 = nodenic_macaddr_p2;
packet.arp.ar_sip = nodenic_ip;
packet.arp.ar_tip = daddr;

/* CHECKSUM_PARTIAL can write L4 csum after we return.
* 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);

@cubesandboxbot cubesandboxbot Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread
jay3cx marked this conversation as resolved.

@cubesandboxbot cubesandboxbot Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the target kernel (5.4), bpf_skb_change_tail() returns -ENOTSUPP for GSO skbs — the codebase already guards this exact call in tcp_reply_reset() with if (skb->gso_segs) return TC_ACT_SHOT; and warns there that bpf_skb_change_tail() "may fail on GSO skbs". The comment here claiming "change_tail also clears any GSO state" isn't accurate for 5.4.

Because redirect_egress() creates the pending direct_neigh entry before calling this function, and that entry rate-limits retries to one per second, a flow that sends only GSO packets to a new on-link destination (e.g. UDP with UFO/GSO from the guest) drops one packet per second forever without ever transmitting an ARP request — the neighbor never resolves and the flow blackholes. Consider checking skb->gso_segs before creating the pending entry and falling back to the gateway-MAC redirect (or dropping without inserting the pending entry) so a later non-GSO packet can trigger resolution.

Comment thread
jay3cx marked this conversation as resolved.
Comment thread
jay3cx marked this conversation as resolved.
if (err)
return TC_ACT_SHOT;
err = bpf_skb_change_head(skb, DIRECT_ARP_HEADROOM, 0);
if (err)
return TC_ACT_SHOT;
err = bpf_skb_store_bytes(skb, 0, &packet, sizeof(packet), 0);
if (err)
return TC_ACT_SHOT;
err = direct_egress_clear_arp_padding(skb);
if (err)
return TC_ACT_SHOT;

return 0;
}

#define EGRESS_MAC_DROP (-1)
#define EGRESS_MAC_READY 0
#define EGRESS_MAC_PROBE 1

/* READY: L2 rewritten. PROBE: skb is now an ARP request. DROP: wait. */
static __always_inline int prepare_egress_l2(struct __sk_buff *skb,
struct ethhdr *l2, __u32 daddr)
{
struct bpf_fib_lookup fib = {
.family = AF_INET,
.ifindex = nodenic_ifindex,
.ipv4_src = nodenic_ip,
.ipv4_dst = daddr,
};
struct direct_neighbor pending = {};
struct direct_neighbor *neighbor;

@cubesandboxbot cubesandboxbot Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

union macaddr *neighbor_mac;
const union macaddr *dmac;
const union macaddr *smac;
__u64 retry_at;
__u64 now;
long err;

if (!direct_egress_is_onlink(daddr)) {
set_mac_pair(l2, egress_smacaddr_p1, egress_smacaddr_p2,
egress_dmacaddr_p1, egress_dmacaddr_p2);
return EGRESS_MAC_READY;
}

err = bpf_fib_lookup(skb, &fib, sizeof(fib),
Comment thread
jay3cx marked this conversation as resolved.
BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT);
if (err == BPF_FIB_LKUP_RET_SUCCESS && fib.ifindex == nodenic_ifindex) {
smac = (const union macaddr *)fib.smac;
dmac = (const union macaddr *)fib.dmac;
set_mac_pair(l2, smac->p1, smac->p2, dmac->p1, dmac->p2);
return EGRESS_MAC_READY;
}

now = bpf_ktime_get_ns();
neighbor = bpf_map_lookup_elem(&direct_neigh, &daddr);

@chenhengqi chenhengqi Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before doing a map lookup, can we try bpf_fib_lookup first?

@jay3cx jay3cx Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fib_lookup first, then ARP if it misses.

@jay3cx jay3cx Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (neighbor) {
if (neighbor->next_probe_at_ns > now) {
if (!direct_neighbor_is_zero(neighbor))
goto set_neighbor;
return EGRESS_MAC_DROP;
}

/* 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;

@cubesandboxbot cubesandboxbot Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Torn MAC exposure: learn_direct_neighbor stores mac->p1 and mac->p2 as two separate stores. A concurrent reader reaching set_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.

  2. Deadline clobber: if this write lands after a concurrent learn, the learned next_probe_at_ns (now + 5 min) is overwritten back down to now + 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.

Comment thread
jay3cx marked this conversation as resolved.
Outdated
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;

@chenhengqi chenhengqi Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simply always do an ARP probe here, right? I am not sure how frequent this concurrent learning will happen.

@jay3cx jay3cx Aug 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Dropped the extra re-lookup and just probe now.

} else {
pending.next_probe_at_ns = now + DIRECT_NEIGH_PROBE_INTERVAL_NS;
err = bpf_map_update_elem(&direct_neigh, &daddr, &pending, BPF_NOEXIST);
if (err)
return EGRESS_MAC_DROP;
}

if (direct_egress_arp_request(skb, daddr))

@cubesandboxbot cubesandboxbot Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return EGRESS_MAC_DROP;
return EGRESS_MAC_PROBE;

Comment thread
jay3cx marked this conversation as resolved.
set_neighbor:
neighbor_mac = (union macaddr *)neighbor->addr;
set_mac_pair(l2, nodenic_macaddr_p1, nodenic_macaddr_p2,
neighbor_mac->p1, neighbor_mac->p2);
return EGRESS_MAC_READY;
}

/*
* Check whether a TCP flow should be redirected to the L7 proxy.
*
Expand Down Expand Up @@ -440,6 +598,7 @@ static __always_inline __u32 do_icmp_nat(struct __sk_buff *skb, struct mvm_meta
struct ethhdr *l2;
struct iphdr *l3;
struct icmphdr *l4;
int mac_result;
__u16 ip_hlen;
__u16 snat_id;
__u64 flags;
Expand Down Expand Up @@ -491,8 +650,11 @@ static __always_inline __u32 do_icmp_nat(struct __sk_buff *skb, struct mvm_meta
icmp_csum_off = ICMP_CSUM_OFF(ip_hlen);

/* update L2 first: csum/store helpers may invalidate packet pointers */
set_mac_pair(l2, egress_smacaddr_p1, egress_smacaddr_p2,
egress_dmacaddr_p1, egress_dmacaddr_p2);
mac_result = prepare_egress_l2(skb, l2, key.dst_ip);
if (mac_result == EGRESS_MAC_DROP)
return 0;
if (mac_result == EGRESS_MAC_PROBE)
return sess->node_ifindex;

/* update ICMP csum: ICMP has no pseudo-header, so no BPF_F_PSEUDO_HDR.
* Only the echo identifier change affects the csum (IP saddr is not
Expand Down Expand Up @@ -543,6 +705,7 @@ static __always_inline __u32 do_udp_nat_inline(struct __sk_buff *skb,
struct ethhdr *l2;
struct iphdr *l3;
struct udphdr *l4;
int mac_result;
__u16 ip_hlen;
__u16 snat_port;
__u64 flags;
Expand Down Expand Up @@ -590,8 +753,11 @@ static __always_inline __u32 do_udp_nat_inline(struct __sk_buff *skb,
udp_csum_off = UDP_CSUM_OFF(ip_hlen);

/* update L2 first: csum/store helpers may invalidate packet pointers */
set_mac_pair(l2, egress_smacaddr_p1, egress_smacaddr_p2,
egress_dmacaddr_p1, egress_dmacaddr_p2);
mac_result = prepare_egress_l2(skb, l2, key.dst_ip);
if (mac_result == EGRESS_MAC_DROP)
return 0;
if (mac_result == EGRESS_MAC_PROBE)
return sess->node_ifindex;

/* update UDP csum only if it was non-zero (UDP csum is optional over IPv4).
* BPF_F_MARK_MANGLED_0 keeps a 0 csum (= disabled) intact in case the
Expand Down Expand Up @@ -685,6 +851,7 @@ static __always_inline __u64 do_tcp_nat(struct __sk_buff *skb, struct mvm_meta *
struct ethhdr *l2;
struct iphdr *l3;
struct tcphdr *l4;
int mac_result;
__u16 ip_hlen;
__u16 snat_port;
__u64 flags;
Expand Down Expand Up @@ -758,8 +925,11 @@ static __always_inline __u64 do_tcp_nat(struct __sk_buff *skb, struct mvm_meta *
tcp_csum_off = TCP_CSUM_OFF(ip_hlen);

/* update L2 first: csum/store helpers may invalidate packet pointers */
set_mac_pair(l2, egress_smacaddr_p1, egress_smacaddr_p2,
egress_dmacaddr_p1, egress_dmacaddr_p2);
mac_result = prepare_egress_l2(skb, l2, key.dst_ip);
if (mac_result == EGRESS_MAC_DROP)
return TCP_NAT_DROP;
if (mac_result == EGRESS_MAC_PROBE)
return TCP_NAT_PACK(sess->node_ifindex, TCP_NAT_OK);

/* update TCP csum: IP saddr is part of pseudo-header, so BPF_F_PSEUDO_HDR */
flags = BPF_F_PSEUDO_HDR | sizeof(old_saddr);
Expand Down
Loading
Loading