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 4 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 all commits
Commits
Show all changes
4 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
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.

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

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.

func Init(params Params) error {

@cubesandboxbot cubesandboxbot Bot Aug 17, 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.

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.

_ = 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.

}

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

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

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

__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
180 changes: 168 additions & 12 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.

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

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

(daddr & nodenic_netmask) == (nodenic_ip & nodenic_netmask);

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

@cubesandboxbot cubesandboxbot Bot Aug 17, 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.

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.

}

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.

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

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

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.

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

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

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.

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

@cubesandboxbot cubesandboxbot Bot Aug 17, 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 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).

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.

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

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;
Comment on lines +242 to +248

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

} 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;

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 @@ -490,10 +648,6 @@ static __always_inline __u32 do_icmp_nat(struct __sk_buff *skb, struct mvm_meta
ip_hlen <<= 2;
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);

/* 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
* covered by ICMP checksum).
Expand Down Expand Up @@ -589,10 +743,6 @@ static __always_inline __u32 do_udp_nat_inline(struct __sk_buff *skb,
ip_hlen <<= 2;
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);

/* 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
* incremental update would yield 0; the helper rewrites it to 0xffff.
Expand Down Expand Up @@ -757,10 +907,6 @@ static __always_inline __u64 do_tcp_nat(struct __sk_buff *skb, struct mvm_meta *
ip_hlen <<= 2;
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);

/* update TCP csum: IP saddr is part of pseudo-header, so BPF_F_PSEUDO_HDR */
flags = BPF_F_PSEUDO_HDR | sizeof(old_saddr);
err = bpf_l4_csum_replace(skb, tcp_csum_off, old_saddr, new_saddr, flags);
Expand Down Expand Up @@ -915,6 +1061,7 @@ int from_cube(struct __sk_buff *skb)
struct iphdr *l3;
struct tcphdr *l4;
struct udphdr *udp;
int mac_result;
__u16 *host_port;
__u32 dns_off;
__u8 proto;
Expand Down Expand Up @@ -1037,6 +1184,15 @@ int from_cube(struct __sk_buff *skb)
return TC_ACT_SHOT;
if (should_redirect_to_l7_proxy(ifindex, daddr, l4))
return bpf_redirect(cubegw0_ifindex, BPF_F_INGRESS);
}

mac_result = prepare_egress_l2(skb, l2, daddr);

@cubesandboxbot cubesandboxbot Bot Aug 17, 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.

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.

if (mac_result == EGRESS_MAC_DROP)
return TC_ACT_SHOT;
if (mac_result == EGRESS_MAC_PROBE)
return bpf_redirect(nodenic_ifindex, 0);

if (proto == IPPROTO_TCP) {
tcp_ret = do_tcp_nat(skb, mvm_meta);
if (TCP_NAT_STATUS(tcp_ret) == TCP_NAT_OK)
return bpf_redirect(TCP_NAT_IFINDEX(tcp_ret), egress_redirect_flags);
Expand Down
Loading
Loading