You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
docs(hrr): document what a capture archive contains and that replay runs its code - #13625
AIRUNTIME-2733 needs the capture privacy guarantee in terms a customer can rely on. The threat model in AIRUNTIME-2853 is an engineering document, not that text. Nothing in the tree says what a shared archive contains. Nothing says that replaying an archive runs the GPU code stored in it. This PR adds that page.
Technical Details
projects/hrr/PRIVACY.md (new) is one page for customers and approvers.
It states that an archive is untrusted executable input. hrr-playback loads the archive's code objects and launches their kernels. The page says how to replay one safely.
It lists each item an archive holds, where it lives, and which capture mode records it. It names the task that removes or reduces each item: AIRUNTIME-2880, AIRUNTIME-2858 or AIRUNTIME-2881.
It lists the manifest device fields and marks uuid, pci, luid and luid_device_node_mask for blanking by AIRUNTIME-2881. The ticket names only UUID and PCI; the LUID fields identify the GPU too.
It lists what capture never reads: host name, command line, environment, user name and time of day.
Event timestamps come from amd::Os::timeNanos. That is CLOCK_MONOTONIC on Linux and QueryPerformanceCounter on Windows, so the page says they are not the time of day.
Every task under the epic is still open. The page therefore describes today's behaviour: one mode, which records contents. Each row names the task that will change it.
The README and the enable-recording skill now link the page.
projects/hrr/tools/tests/test_privacy_doc.py (new) keeps the page current with eight tests:
the device-field table names exactly the keys hip_capture_metadata.cpp writes;
the inventory lists the active marker named in hip_capture_writer.cpp;
the inventory has a blobs/ row for hipHostRegister, whose wrapper writes a blob;
the inventory has a blobs/ row for hipLinkAddData saying rejected inputs are kept, while the wrapper does not check the result;
every shim that copies a caller's hipDeviceProp_t (today hipChooseDevice) has an events.bin row and is a listed string source;
the page keeps the untrusted-input statement;
the page links every epic task;
the README and the skill link the page.
The test runs under the existing ctest hrr-generator-rules. The comment in projects/hrr/tests/CMakeLists.txt now says so. No CI job runs tests/scripts, so the test is not there.
Issue Tracking
JIRA ID: AIRUNTIME-2884
Test Plan
HRR build with capture compiled into CLR, unit and integration ctest, then every Catch2 case one at a time, hidden cases included, on MI350X (gfx950). Head c597948c2e. The later commits up to f19e95b12d, the current head, change only PRIVACY.md and test_privacy_doc.py; they were checked locally.
Negative control: this branch with PRIVACY.md deleted and the README and skill links reverted, same gate.
Baseline: develop at 839470366c, the base of this branch, same runner and same scripts.
Locally, on head f19e95b12d: python3 -m unittest discover -s projects/hrr/tools/tests, pytest on the enable-recording skill, and the skillscope structural check at the pin hrr-skills.yml uses.
Mutation check: one more manifest field in hip_capture_metadata.cpp makes the new test fail.
Every gate case has the same status on all three runs. The 23 hidden cases that do not pass also fail on develop: 20 fail, 2 crash with SIGSEGV and 1 with SIGABRT.
The gate's ctest step runs only hrr-unit-tests, not hrr-generator-rules. The gate therefore never ran the new test, which is why the negative control passes there. The new test ran only locally. On the negative-control tree all eight of its tests fail.
The page's claims about future modes are untested. They depend on AIRUNTIME-2880, AIRUNTIME-2858 and AIRUNTIME-2881 landing as their tickets describe. The manifest table's field names and the inventory rows for the active marker, hipHostRegister, hipLinkAddData and hipChooseDevice are checked against code. The Kept and Removed-by columns and the other rows are not.
…uns its code
AIRUNTIME-2733 needs the privacy guarantee in terms a customer can rely on,
separate from the engineering threat model. PRIVACY.md is that page: it lists
what an archive holds today, which capture mode records each item once the
metadata-only mode lands, which task under the epic removes or reduces it,
what capture never reads, and that an archive is untrusted executable input
because replay launches the GPU code stored in it.
The README and the enable-recording skill link the page. A unittest in
tools/tests, run by the hrr-generator-rules ctest, fails when the device
fields hip_capture_metadata.cpp writes into the manifest and the page's table
disagree, so a task that adds or blanks a field has to update the page.
Jira: AIRUNTIME-2884
…tring rows
Replay translates only values that match a recorded allocation, so kernel
arguments can still carry the original process's addresses. The inventory
now lists the hipHostRegister snapshot and the active marker, whose Windows
form holds the process creation time, and the string list names the direct
string arguments of HIP calls and registered variable names. The privacy
test checks the two new rows against the capture sources.
Linker input buffers are missing from the capture inventory
projects/hrr/PRIVACY.md:52
The inventory omits linker input buffers. capture_hipLinkAddData (projects/clr/hipamd/src/hrr/hip_capture.cpp:2318–2323) writes all size bytes of a non-null input to blobs/, even when the HIP call fails. Rejected bitcode or inputs never linked or loaded can therefore disclose code and private paths without fitting the existing rows. Add a contents-mode row for these buffers, including rejected inputs, and include them in the string-source list under “Never recorded.”
capture_hipLinkAddData writes every byte of a non-null input to blobs/
whatever the call returns, so rejected inputs and inputs never linked are
recorded too. The page now has a contents row for them and names them as a
string source; the privacy test checks the row and that the wrapper does not
gate on the result.
Linker input buffers are missing from the capture inventory: fixed in 30cb8f6ba5, which adds a contents row for hipLinkAddData inputs (every byte, rejected and never-linked inputs included), names them as a string source under Never recorded, and tests the row against the wrapper.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The customer-facing inventory omits input-structure snapshots that can retain device identifiers outside the manifest.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
HIP input snapshots omitted from inventory and exclusive string-source lists
projects/hrr/PRIVACY.md:46
The inventory and the exclusive string-source list below omit HIP input-structure snapshots. capture_hipChooseDevice copies the entire supplied device-properties struct into events.bin (hip_capture_generated.cpp:201–212), including character arrays and UUID/LUID/PCI fields. The integration workload passes actual device properties to this call (hrr_workload_test.cc:1323–1327), so these identifiers can occur outside manifest.json without a buffer transfer or registration. Add these snapshots to both lists and clarify their planned capture-mode and identifier treatment; blanking manifest fields alone will not remove these copies.
The reason will be displayed to describe this comment to others. Learn more.
Review — no blocking findings
Full review of the whole change (origin/develop...0da567c6). Implements AIRUNTIME-2884 under AIRUNTIME-2733 (not retrievable from here; read the PR description only).
Blocking
None
Non-blocking
The page says an unmatched pointer argument "is passed as null", but replay passes non-address values through unchanged — projects/hrr/PRIVACY.md ("Replay runs GPU code…", the sentence beginning "Only a value that matches a recorded allocation is translated") playback/hip_playback.cpp (whole-arg pointer branch, ~L795-805): a value that is not VA-shaped is treated as a sentinel and live = rec_ptr is kept; only VA-shaped unresolved values become null. The next sentence ("every other argument byte… is passed as recorded") is right, so the two sentences disagree and the code governs. Say "a VA-shaped pointer argument that matches none is passed as null; a value that cannot be an address is passed as recorded".
Confidence 85 · mechanical · Fix: reword as above.
A customer-facing page links internal Jira tickets — PRIVACY.md (header, Capture modes, inventory table, last section), tools/tests/test_privacy_doc.py (EPIC_TASKS, test_links_each_epic_task)
The page calls itself the customer text, yet its only pointers for "what removes this" are amd-hub.atlassian.net tickets a customer cannot open, in a public repo. The test then pins those URLs, so removing them later needs a test change. Prefer a plain description of the planned behaviour (keep the ticket ids in the PR/commit, not the shipped page), or confirm the hub is world-readable.
Confidence 70 · architectural · Fix: drop the URLs from the page and the link test.
The "Mode" column and "Capture modes" section document a mode that does not exist yet — PRIVACY.md
Every row's both/contents claim is about code not in the tree and is untested (the PR description says so). That is a maintenance liability if AIRUNTIME-2880 lands differently. Consider keeping the page to today's behaviour and adding the column when the mode lands.
Confidence 70 · architectural · Fix: author's call (see Decisions).
The page's promise that the test "fails when the manifest fields… and the table disagree" covers names only — PRIVACY.md intro, test_privacy_doc.py:device_fields_documented
The Kept / Removed-by columns are not checked, so a field flipping from kept to blanked leaves the test green while the page is wrong. Mutation check by reading: only the first column is parsed.
Confidence 70 · logic · Fix: also assert uuid/pci/luid/luid_device_node_mask rows are the "no" rows, or soften the intro sentence.
PR description is stale against the final branch — PR body
It says four new tests and "only the manifest table is checked against code", but the file has eight, and inventory rows (active marker, hipHostRegister, hipLinkAddData, hipChooseDevice) are checked too; the test-plan head c597948c2e is not the current head. Update the description.
Confidence 85 · mechanical · Fix: edit description.
Decisions for the author
Forward-looking Mode column vs. today-only page — tradeoff
Forward-looking helps approvers see the end state; today-only cannot go stale ahead of the code.
Docs-only checks live in hrr-generator-rules, which the CI gate does not run — tradeoff
The description says so; the test is a local-only guard until a job runs it.
Positive signals
The untrusted-executable-input section is blunt and actionable (trust the producer, unprivileged user, container, --info needs no GPU).
Tests tie the page to code: device keys parsed from append_prop_fields, kActiveMarker, the un-gated hipLinkAddData wrapper.
Deployment notes
None
What this covered
All five files completely, plus hip_capture_metadata.cpp, hip_capture.cpp (hipHostRegister/hipLinkAddData), hip_capture_writer.cpp (marker) and the pointer-argument path in hip_playback.cpp. Ran test_privacy_doc.py against the PR tree extracted to a scratch directory: 8 of 8 pass. CI: no failing checks. Single-context pass (change is small); not a fan-out, so independent coverage is lower than a split review. No mutation runs. Ticket AIRUNTIME-2733 not retrieved. History/prior-changes passes not run. Not reconciled against any discussion here.
The reason will be displayed to describe this comment to others. Learn more.
Re-checked f19e95b: the pointer-argument sentence now matches hip_playback.cpp:805 (only a VA-shaped unmatched pointer becomes null, a sentinel passes through), and the description now names eight tests and the current head. Still non-blocking, your call: projects/hrr/PRIVACY.md - the 14 amd-hub.atlassian.net links are not openable by customers; projects/hrr/tools/tests/test_privacy_doc.py - also assert the Kept column for the blanked device fields.
Copilot's previously missed finding on PRIVACY.md:46 (HIP input snapshots missing from the inventory and string-source lists) is fixed in 0da567c. The inventory has an events.bin row for the hipDeviceProp_t that hipChooseDevice copies, with its name, UUID, LUID and PCI fields, and a row for the other input structures copied whole, padding included. The string-source list names both. test_privacy_doc.py finds every capture function that copies a hipDeviceProp_t and requires it on the page; it fails without the page change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
danielbolsson
left a comment
Motivation
AIRUNTIME-2733 needs the capture privacy guarantee in terms a customer can rely on. The threat model in AIRUNTIME-2853 is an engineering document, not that text. Nothing in the tree says what a shared archive contains. Nothing says that replaying an archive runs the GPU code stored in it. This PR adds that page.
Technical Details
projects/hrr/PRIVACY.md(new) is one page for customers and approvers.hrr-playbackloads the archive's code objects and launches their kernels. The page says how to replay one safely.uuid,pci,luidandluid_device_node_maskfor blanking by AIRUNTIME-2881. The ticket names only UUID and PCI; the LUID fields identify the GPU too.amd::Os::timeNanos. That isCLOCK_MONOTONICon Linux andQueryPerformanceCounteron Windows, so the page says they are not the time of day.projects/hrr/tools/tests/test_privacy_doc.py(new) keeps the page current with eight tests:hip_capture_metadata.cppwrites;activemarker named inhip_capture_writer.cpp;blobs/row forhipHostRegister, whose wrapper writes a blob;blobs/row forhipLinkAddDatasaying rejected inputs are kept, while the wrapper does not check the result;hipDeviceProp_t(todayhipChooseDevice) has anevents.binrow and is a listed string source;hrr-generator-rules. The comment inprojects/hrr/tests/CMakeLists.txtnow says so. No CI job runstests/scripts, so the test is not there.Issue Tracking
JIRA ID: AIRUNTIME-2884
Test Plan
PRIVACY.mdandtest_privacy_doc.py; they were checked locally.PRIVACY.mddeleted and the README and skill links reverted, same gate.python3 -m unittest discover -s projects/hrr/tools/tests, pytest on the enable-recording skill, and the skillscope structural check at the pinhrr-skills.ymluses.hip_capture_metadata.cppmakes the new test fail.Test Result
tools/testsunittest (local)Every gate case has the same status on all three runs. The 23 hidden cases that do not pass also fail on develop: 20 fail, 2 crash with SIGSEGV and 1 with SIGABRT.
The gate's ctest step runs only
hrr-unit-tests, nothrr-generator-rules. The gate therefore never ran the new test, which is why the negative control passes there. The new test ran only locally. On the negative-control tree all eight of its tests fail.The page's claims about future modes are untested. They depend on AIRUNTIME-2880, AIRUNTIME-2858 and AIRUNTIME-2881 landing as their tickets describe. The manifest table's field names and the inventory rows for the
activemarker,hipHostRegister,hipLinkAddDataandhipChooseDeviceare checked against code. The Kept and Removed-by columns and the other rows are not.Submission Checklist