oxidecomputer/opte

Inbound external packets contain uninitialised mblk data in `cargo test`

Closed this issue · 6 comments

FelixMcFelix commented

Updating to a newer rust version in #453 has turned up an issue where inbound packets can contain bonus data after processing. Replicating evidence from the issue:

cargo -V Csum bytes
cargo 1.75.0 (1d8b05cdd 2023-11-20) [0x3d, 0xac]
cargo 1.76.0 (c84b36747 2024-01-18) <different every time, random>
cargo 1.77.0-nightly (2ce45605d 2024-01-04) [0x3d, 0x2c]
cargo 1.78.0-nightly (cdf84b69d 2024-02-02) <different every time, random>

DATA_VEC below refers to all bytes, up to current recorded len, copied from Packet.segs into a single Vec<u8>, indexewd from the start of the ICMP header. The inbound rewritten packets on 1.75 have a trailer of all zeroes:

[3d, ac]
DATA_VEC? [0, 0, 3d, ac, 0, 7, 3, 9, 72, 65, 75, 6e, 69, 6f, 6e, 0, 0, 0, 0, 0, 0, 0, 0, 0]

while those on other compiler versions have garbage data:

[3d, ac]
DATA_VEC? [0, 0, 3d, ac, 0, 7, 3, 9, 72, 65, 75, 6e, 69, 6f, 6e, 0, 0, 0, 0, 80, f0, 5c, df, ea]

Digging into the pseudo mblk_ts, these are init'ed using Vec::with_capacity in user/test code (so contents are not zero-initialized). I spent some time looking at mblk body addresses and I didnt directly see any historic overlaps, but we're clearly overlapping some older region of the heap.

The fastest fix is probably to more aggressively trim output packets in Direction::In -- I assume we're specifically 8 bytes over, at least in the ICMP case. I think the more correct longer-term fix is to be more honest in Packet around what it means to expand a packet segment. We obviously don't want to zero-fill a buffer, but we should probably be incrementing the wptr after a write into a MaybeUninitialized region, rather than incrementing wptr and handing out &[u8]s into that memory.

FelixMcFelix commented

This may be on all inbound packets. Looking at an inbound TCP SYN+ACK from another test case:

[
// 0, ethernet (len 14)
0xa8, 0x40, 0x25, 0xfa, 0xfa, 0x37,
0xa8, 0x40, 0x25, 0xff, 0x77, 0x77,
0x8, 0x0,
// 14, Ipv4 (20B), => total len 40B
0x45, 0x0, 0x0, 0x28,
0xa, 0x66, 0x40, 0x0,
0x40, 0x6, 0xcf, 0xf7,
0x34, 0xa, 0x80, 0x45,
0xac, 0x1e, 0x0, 0x5,
// 34, tcp (no opts, => 16B)
0x0, 0x50, 0xad, 0xca,
0x2, 0xa1, 0xd9, 0x47,
0x8d, 0xfc, 0x28, 0xd4,
0x50, 0x12, 0x0, 0x0,
0xe, 0x8c, 0x0, 0x0,
// no payload.
// 8 byte trailer 😱
0, 0, 0, 0, 0, 0, 0, 0
]
rcgoodfellow commented

Just an observation that these are very short packets, and we could be hitting a minimum packet size issue with the underlying NIC/hardware.

rcgoodfellow commented

This comes to mind

luqmana commented

I think the more correct longer-term fix is to be more honest in Packet around what it means to expand a packet segment.

Yea, the mblk_t/Packet abstraction leave much to be desired :/

FelixMcFelix commented

Just an observation that these are very short packets, and we could be hitting a minimum packet size issue with the underlying NIC/hardware.

I'm hoping this is only happening in the test harness, but that makes it doubly strange since we don't need to hit the 64B minimum ethernet packet size with the mocked up mblk_ts -- everything should be exactly sized when we go to process. I'll dig in more on Monday.

FelixMcFelix commented

I'm not seeing this via snoop on any of the dogfood nexus zones with inbound packets, so I believe this is confined to the test suite?

I am seeing the same thing with longer external packets, but not guest-to-guest traffic. Total expected length would be 88B so it's not confined to short packets:

// 96 total -- expect 88
[
    // 00-13 eth
    0xa8,0x40,0x25,0xfa,0xfa,0x37,
    0xa8,0x40,0x25,0xff,0x77,0x77,
    0x08,0x00,
    // 14-33 ip (len 0x4a=74, tcp+payl=54)
    0x45,0x00,0x00,0x4a,
    0x00,0x00,0x40,0x00,
    0x40,0x06,0xda,0x3b,
    0x34,0x0a,0x80,0x45,
    0xac,0x1e,0x00,0x05,
    // 34-53 tcp
    0x00,0x50,0xad,0xca,
    0x02,0xa1,0xd9,0x49,
    0x8d,0xfc,0x28,0xe6,
    0x50,0x18,0x00,0x00,
    0x78,0x31,0x00,0x00,
    // 54+=(34) payl
    0x48,0x54,0x54,0x50,
    0x2f,0x31,0x2e,0x31,
    0x20,0x33,0x30,0x31,
    0x20,0x4d,0x6f,0x76,
    0x65,0x64,0x20,0x50,

    0x65,0x72,0x6d,0x61,
    0x6e,0x65,0x6e,0x74,
    0x6c,0x79,0x0d,0x0a,
    0x0d,0x0a,

    // Mystery 8 random bytes
    0x01,0x00,0x7f,0xfc,
    0xff,0x00,0xb8,0xc6,
]
  • 👍1