Repository navigation
UCT/IB: Dump complete packets in trace-data logs - #11975
Conversation
uct_ib_log_dump_sg_list() gathered the packet into a fixed char data[256] before handing it to the dump callback. The callbacks parse the message by its own headers and are not given a length they can rely on, so a message longer than the copy was parsed past its end. A wireup request, which is several hundred bytes, was reported as "WIREUP REQ [<malformed address>]". Size the gather buffer to the message instead, using ucs_alloc_on_stack() so that only large packets reach the heap. Skip the callback when no data was collected, since it has no header to read in that case. A description can still be cut by the length of the log line, so stamp the end of the line with an ellipsis. A description which fits keeps its own terminator and hides the stamp behind it.
|
🤖 Starting review — findings will be posted here when done. |
|
is this path exercised anywhere in CI? gtest runs with |
- Cap the gathered dump length by the iface segment size, so that a traced packet does not allocate and copy an entire zcopy message - Drop the unused valid_length argument from the packet dump callbacks - Use UCT_IB_LOG_LINE_LEN in uct_ib_mlx5_cqe_dump() - Add a gtest which sends am_zcopy with trace-data logging enabled, covering both the gather path and the line cut
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — [incomplete: stop_reason=max_tokens] |
- Fit am_zcopy header and payload in max_zcopy, which is 1024 on some setups, so the send is not rejected - Protect the captured log lines with the logger mutex, since trace-data is also emitted from the async progress thread
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage note: the new test covers only Residual coverage note: |
Skip the gather when a single segment is dumped, which is the case for every IB transport, so the common path passes the segment as it is and allocates nothing. Keep the sized gather for the multi-segment case, so a message larger than the stack limit is still dumped in full.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN GPU job failed on a single unrelated gtest, Full analysisSummary: Root cause: Evidence from the log: the only failure is in the multi-threaded memtype-cache test; no ASAN report, no crash, no timeout (the suite ran to completion and tore down normally, largest inter-line gap is a few seconds). Reading the test,
Implicated commit: unknown — not the PR head File: test/gtest/ucs/test_memtype_cache.cc:572 ( Suggested fix: Re-run the Related: none found (no existing issue/PR matches |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The GPU gtest job failed on a single protocol-selection assertion — Full analysisSummary: Root cause: The failing test ( Implicated commit: Not from this PR. Most likely File: test/gtest/ucp/test_ucp_proto.cc:805 (test body), with the failing assertions at test/gtest/ucp/test_ucp_proto.cc:438 and test/gtest/ucp/test_ucp_proto.cc:457 Suggested fix: 1) Re-run the gpu job on PR #11975 (and on a bare master build of the same container/GPU node) to confirm the failure is not caused by the PR — expect it to reproduce without the wireup change. 2) If it reproduces on master, fix the test rather than the PR: in |
What?
uct_ib_log_dump_sg_list()copies the packet into a fixedchar data[256]before handing it to the dump callback, which then parses past the end of the copy for any larger packet. That is an out-of-bounds read of the stack buffer, and it prints a bogus description. Only reachable with trace-data logging enabled (UCX_LOG_LEVEL=dataor more verbose); nothing is wrong on the wire.A 325-byte
WIREUP REQ, before and after:Why?
The dump callbacks parse a message by its own headers and get no length they can rely on —
ucp_address_unpack()takes none at all — so a truncated copy makes them walk off its end. Only the send path was affected; receive completions pass the real buffer through.How?
Size the gather buffer to the message with
ucs_alloc_on_stack(), so only large packets reach the heap. Skip the callback when nothing was collected, since it has no header to read. A description can still be cut by the log line length, so stamp the line end with an ellipsis; one that fits keeps its own terminator and hides the stamp. Verified on ConnectX with loopbackucx_perftestand therc_mlx5/ud_mlx5p2p gtests underUCX_LOG_LEVEL=data.