Read SP task crash dumps over the network #1921

mkeeter · 2024-11-18T15:19:50Z

Hubris counterpart to oxidecomputer/management-gateway-service#316, allowing us to read task dumps over the network via MGS.

These MGS messages – unlike dump-agent – require more intelligence into the Hubris side. The external API is just "start read" and "continue read"; Hubris is responsible for knowing how humpty writes stuff into RAM.

We allow for up to 4 simultaneous readers, distinguished by caller-provided keys. If there are more than four simultaneous readers, then (1) what the heck, and (2) some readers will return an error indicating that their key is no longer valid.

There are also minor tweaks to dump-agent and jefe implementations:

dump-agent learns how to read dump data into a lease (instead of returning a 256-byte value)
jefe caches the most recent dump area → memory address lookup, so we don't always need to start from the beginning of the linked list. This isn't useful if we have multiple readers, but makes the single-reader case less Accidentally Quadratic.

labbott

I'm a little bit iffy about failure modes for more than 4 clients. It seems like the only result should be bad data on the client end? A sufficiently malicious client would potentially only cause a DoS?

app/gimlet/base.toml

labbott · 2024-11-18T19:11:26Z

task/jefe/src/dump.rs

@@ -30,6 +30,7 @@ enum Trace {
    GetDumpArea(u8),
    Base(u32),
    GetDumpAreaFailed(humpty::DumpError<()>),
+    GetDumpAreaFailed2(Result<DumpArea, DumpAgentError>),


Perhaps a different name? DumpAreaAgentFailure?

Oops, this was for debugging; removed

hawkw · 2024-11-18T21:33:48Z

task/control-plane-agent/src/dump.rs

+    offset: u32,
+}
+
+const MAX_DUMP_CLIENTS: usize = 4;


why 4? is this to allow two MGSes, plus...two more MGSes?

Mostly vibes, but I was thinking something like "max of two MGSes plus two tech port connections".

hawkw · 2024-11-18T21:55:50Z

task/control-plane-agent/src/dump.rs

+        for index in 0.. {
+            // XXX accidentally quadratic!
+            let data = self
+                .agent
+                .read_dump(index, 0)
+                .map_err(|_e| SpError::Dump(DumpError::BadArea))?;
+            let header = humpty::DumpAreaHeader::read_from(
+                &data[..size_of::<humpty::DumpAreaHeader>()],
+            )
+            .unwrap_lite();
+            if header.contents == humpty::DumpContents::SingleTask.into()
+                && header.nsegments > 0
+            {
+                count += 1;
+            }
+            if header.next == 0 {
+                break;
+            }
+        }


I wonder whether it would make sense to have the dump agent just have a counter of the number of single-task dumps, so that we could just ask it for that, instead of probing every dump area? Looking at the dump-agent implementation, that might be somewhat difficult, as reinitialize_dump_from would have to read the reinitialized memory in order to reason about how many single-task dumps are being discarded...maybe this is the better approach?

hawkw · 2024-11-18T22:01:12Z

task/control-plane-agent/src/dump.rs

+        }
+
+        // Move along to the next area if we're at the end
+        if state.offset + size_of::<humpty::DumpSegmentData>() as u32


considered obnoxious, feel free to ignore: size_of::<humpty::DumpSegmentData>() and size_of::<humpty::DumpAreaHeader>() are a little bit hard to tell apart in this function, because they're both long-ish expressions and the type parameter to size_of starts with humpty::Dump and my eyes glazed over at that point. I might consider doing something like

const HEADER_SIZE: u32 = size_of::<humpy::DumpAreaHeader>() as u32; const SEGMENT_DATA_SIZE: u32 = size_of::<humpy::DumpSegmentData>() as u32;

and then referring to them by those constants elsewhere, so that it's more obvious that they're different things? but maybe this is just a "me thing", in which case you're welcome to ignore me.

Sure, done in 1ddd388

hawkw · 2024-11-18T22:06:40Z

task/jefe/src/main.rs

    #[cfg(feature = "dump")]
    dump_areas: u32,
+
+    /// Cache of most recently checked dump area


might be nice to mention what this is used for?

task/jefe/src/main.rs

hawkw · 2024-11-18T22:10:27Z

task/jefe/src/main.rs

+                    if index == prev.index {
+                        // Easy case: we've already looked up this area
+                        Ok(prev)
+                    } else if let Some(offset) = index.checked_sub(prev.index) {


i was going to suggest that this expression might be better represented as a match, since the two default cases could be combined, but...i think you can't write a match guard like

Some(prev) if let Some(offset) = index.checked_sub(prev.index) =>

which is a bit of a bummer. I suppose you could instead write

Some(prev) if prev.index >= index =>

and then do the subtraction in the body of the match arm, but using checked_sub both as the check and the actual subtraction feels much nicer, and the case where the checked_sub returns None would still have to be handled despite never actually happening, which feels quite sad...

mkeeter · 2024-11-20T16:21:10Z

I'm a little bit iffy about failure modes for more than 4 clients. It seems like the only result should be bad data on the client end? A sufficiently malicious client would potentially only cause a DoS?

Yeah, that's correct – if I open 5x terminals and run

faux-mgs --log-level=debug --interface en0 dump read --index 0

in all of them, the first one stops with Error: Error response from SP: dump: invalid key

bcantrill

Looks great, modulo very small comments. Will push the fix to humpty now to allow you to use the proper constant.

Cargo.toml

task/control-plane-agent/src/dump.rs

See `oxidecomputer/management-gateway-service#316` and `oxidecomputer/hubris#1921` for details This PR adds a new `humility hydrate` subcommand, which takes a "dry" task dump and staples it to a Hubris archive, creating a normal Hubris core file.

mkeeter requested review from cbiffle, bcantrill, jgallagher and hawkw November 18, 2024 15:19

mkeeter mentioned this pull request Nov 18, 2024

Add subcommand to "hydrate" a task dump oxidecomputer/humility#520

Merged

mkeeter force-pushed the mkeeter/mgs-task-dumps branch from 16702f0 to 7ff43aa Compare November 18, 2024 15:25

labbott reviewed Nov 18, 2024

View reviewed changes

mkeeter force-pushed the mkeeter/mgs-task-dumps branch from 6735862 to f93d5eb Compare November 18, 2024 19:24

hawkw reviewed Nov 18, 2024

View reviewed changes

mkeeter force-pushed the mkeeter/mgs-task-dumps branch 2 times, most recently from 7a7119f to 4c7a97f Compare November 20, 2024 16:30

mkeeter force-pushed the mkeeter/mgs-task-dumps branch from 4c7a97f to c9421c6 Compare December 6, 2024 21:22

bcantrill approved these changes Dec 10, 2024

View reviewed changes

mkeeter force-pushed the mkeeter/mgs-task-dumps branch from 5d4b929 to d32a1b4 Compare December 10, 2024 19:56

mkeeter added 15 commits December 10, 2024 15:07

Add helper function to read dump data into a lease

077b380

Cache dump area lookup

e49195e

Implement reading task crash dumps over MGS

6765f85

Switch to upstream MGS

c9a0949

Fix priorities

31fe933

Clippy fixes

8cc044f

Fix LPC55xpresso build

1ef84cb

Remove debug trace

692313c

Fix typo

186f89b

Add comment about accelerator

24ce008

Add constants for size_of stuff

0131baa

Support UUID + sequence number

bfb549f

Move clear_client_state call to a single place

fde6b2e

Fix sidecar and PSC build

9ec159d

Updates per review

23cedec

Switch back to gateway-messages master

a7ef633

mkeeter force-pushed the mkeeter/mgs-task-dumps branch from d32a1b4 to a7ef633 Compare December 10, 2024 20:07

mkeeter merged commit 1757b8b into master Dec 10, 2024
125 checks passed

mkeeter deleted the mkeeter/mgs-task-dumps branch December 10, 2024 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read SP task crash dumps over the network #1921

Read SP task crash dumps over the network #1921

mkeeter commented Nov 18, 2024

labbott left a comment

labbott Nov 18, 2024

mkeeter Nov 18, 2024

hawkw Nov 18, 2024

mkeeter Nov 19, 2024

hawkw Nov 18, 2024

hawkw Nov 18, 2024

mkeeter Nov 20, 2024

hawkw Nov 18, 2024

mkeeter Nov 19, 2024

hawkw Nov 18, 2024

mkeeter commented Nov 20, 2024

bcantrill left a comment

Read SP task crash dumps over the network #1921

Read SP task crash dumps over the network #1921

Conversation

mkeeter commented Nov 18, 2024

labbott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeter commented Nov 20, 2024

bcantrill left a comment

Choose a reason for hiding this comment