Repository navigation
ocp: Fix OCP telemetry timestamp field decode - #2
Closed
jeff-lien-sndk wants to merge 263 commits into
Closed
jeff-lien-sndk wants to merge 263 commits into
jeff-lien-sndk wants to merge 263 commits into
Conversation
jeff-lien-sndk
requested review from
brandon-paupore-sndk
and
a lite review from Copilot
October 12, 2026 21:14
There was a problem hiding this comment.
🟡 Changes recommended
The existing telemetry tests would not catch a return to the incorrect eight-byte decode.
1 open finding
What changed in this PR
This PR corrects how the OCP telemetry parser reads timestamps, excluding the attribute and reserved bytes from the decoded value.
Changes:
- Decode the timestamp from its six-byte value using the NVMe timestamp structure.
| File | Description |
|---|---|
src/field-parser.c |
Uses the 48-bit timestamp value when formatting OCP telemetry fields. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| struct nvme_timestamp *ts = | ||
| (struct nvme_timestamp *)&buf[offset]; | ||
|
|
||
| shr_format_ts(int48_to_long(ts->timestamp), ts_buf); |
Collaborator
There was a problem hiding this comment.
Yeah, updating the testcase seems like it'd be a good idea.
There was a problem hiding this comment.
Added nonzero timestamp and attribute/reserved bytes to the fixture, with expected formatted output asserted in both text and JSON modes. Commit: 9814c2d.
brandon-paupore-sndk
left a comment
brandon-paupore-sndk
left a comment
Collaborator
There was a problem hiding this comment.
Changes look mostly good to me, just a few things that might be worth tidying up
| struct nvme_timestamp *ts = | ||
| (struct nvme_timestamp *)&buf[offset]; | ||
|
|
||
| shr_format_ts(int48_to_long(ts->timestamp), ts_buf); |
Collaborator
There was a problem hiding this comment.
Yeah, updating the testcase seems like it'd be a good idea.
jeff-lien-sndk
force-pushed
the
timestamp-decode-fix
branch
5 times, most recently
from
October 13, 2026 21:05
6787d40 to
ad36da0
Compare
parse_numeric_addr() writes a struct sockaddr_in or sockaddr_in6 into a struct sockaddr_storage, and sockaddrs_eq() reads it as a struct sockaddr. This breaks the strict aliasing rule: the compiler may assume that the write does not change what the read returns. The bug was latent. An unrelated change in this file made gcc 14 inline differently in a musl release build. The generated code then compared two equal IPv4 addresses as different. Access the addresses through a union of the sockaddr types, and copy the addresses of getifaddrs() into it. Fixes: 74b5d63 ("libnvme/util: reimplement numeric address helpers without getaddrinfo") Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Every interface has a route to the IPv6 link-local prefix, so the route lookup for a link-local traddr returns any interface. The I/O queue count then comes from the wrong NIC. For a link-local destination, return the interface that the scope names. Accept an interface index as scope too, as the kernel does. Fixes: 6153710 ("shared/net-util-linux: add support for retrieving egress netdev") Signed-off-by: Martin Belanger <martin.belanger@dell.com>
The Supported Capacity Configuration List log has no length field, so it is read again with a larger buffer until it holds all the descriptors. The Media Unit Status log needs the same loop. Move the loop to a helper. The helper reads with libnvme_get_log_dynamic_chunk(), and the maximum length is raised from 256 KiB to 4 MiB. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Only the 16-byte log header was read. The printers then read the descriptors from memory past the end of the buffer. They indexed the descriptors as an array of 14-byte structs, but a descriptor is CIO + 2 * MUCS bytes long. So every descriptor after the first one was read at a wrong offset. The JSON printer used NMU without converting it from little endian, and the binary printer dumped only the header. Read the log again with a larger buffer until it holds all the descriptors. Pass the length to the printers and stop at the end of the buffer, or at a descriptor whose CIO points into its own header. Also print the Channel Identifiers. Fixes: linux-nvme#4155 Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Expose the namespace wwid through the generated lazy accessors and use it in the topology test instead of the legacy uuid sysfs attribute, which warns when falling back to the NGUID. Keep the existing identifier accessors unchanged. Signed-off-by: ooonea <35407790+ooonea@users.noreply.github.com> Assisted-by: OpenAI Codex
nvme-discoverd reads discovery logs in its event loop, and a read waits for the DC. The resolver already runs getaddrinfo() in worker threads. Move its thread pool into dmn_work, which runs any function in a worker and reports its end from the event loop. The resolver uses dmn_work and keeps its API. A timeout now counts from the current time. sd_event_now() is the time of the last wakeup of the event loop, so a timeout started at the end of a long callback expired too early. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Reading a discovery log blocks until the DC answers. A DC that does not answer blocked the event loop until the kernel admin timeout, 60 seconds by default: no events, no D-Bus, no other DC. Read the log in a worker, with a libnvme global context of its own, and process it in the event loop. One read runs at a time per DC. A change event during a read reads the log again when the read ends. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
The hidden global --human-readable/-H is kept for backwards compatibility as a spelling of --verbose, but it was an OPT_FLAG which stores a bool into the unsigned int nvme_args.verbose. That only writes the first byte of the counter: on big-endian machines -H sets verbose to 0x01000000, and on little-endian machines it resets an already counted -vv back to 1. Declare it as OPT_INCR like --verbose and make the counter an int, which is the type CFG_INCREMENT stores. Reject verbose values from nvme-cli.conf which do not fit. Signed-off-by: Daniel Wagner <dwagner@suse.de>
All OPT_LONG options store into __u64 or uint64_t variables (offsets, sizes, a timestamp, a component id), but CFG_LONG writes an unsigned long. That is only 32 bits wide on Windows and 32-bit targets, so the upper half of the variable is never written and values above 4 GiB are truncated. Parse with strtoull() and store a uint64_t. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The value of set-property is a __u64, since properties can be 8 bytes wide, but it was parsed with OPT_UINT which only stores 32 bits. The variable is initialized to -1 to detect a missing value, so the upper half stayed all ones and an 8-byte property was set to 0xffffffff00000000 | value. Use OPT_SUFFIX like set-reg does for its 64-bit value. Signed-off-by: Daniel Wagner <dwagner@suse.de>
OPT_INT stores an int, but the tls_key_id variable is a long, so only the first four bytes are written. On big-endian machines the value lands in the upper half of the long and the key id is lost. Making the variable an int does not lose any key ids: - The kernel key serial is a key_serial_t, which is an int32_t, and serials are always positive. - OPT_INT already parsed the value into an int, so nothing wider was ever accepted. - libnvmf_context_set_tls_key_id() takes a long, so the int is widened on the call, and libnvme narrows it back to an int when it builds the connect string (__add_hex_argument()), skipping negative values. On little-endian machines the upper half of the zero-initialized long stayed zero, so the same 32 bits reached the kernel before and after this change. A TLS connection to an nvmet target showed byte-identical connect strings with and without this change for decimal, hex, negative and out-of-range --tls_key values, and the key given by its serial was the one used for the handshake. Signed-off-by: Daniel Wagner <dwagner@suse.de>
set-latency-monitor-feature parses every option with OPT_UINT, which stores 32 bits, but the fields are __u8 and __u16. Each option overwrites its neighbours, depending on the order on the command line, and latency_monitor_feature_enable, the last field, writes 3 bytes past the end of the struct. Use OPT_BYTE and OPT_SHRT, which also reject values that do not fit. The interval of set-plp-health-check-interval is a 16-bit host order value which was stored with OPT_BYTE into an __le16; make it an __u16 parsed with OPT_SHRT. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The latency monitor command has the same OPT_UINT into __u8/__u16 overflow as the OCP version, writing past the end of its config struct. The vs-smart-add-log interval is a uint8_t parsed with OPT_UINT and the drive resize size is a uint64_t of which only 32 bits were written. Use OPT_BYTE, OPT_SHRT and OPT_LONG matching the field widths. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The vs-smart-add-log interval is a uint8_t parsed with OPT_UINT, which writes 32 bits, and the drive resize size is a uint64_t of which only 32 bits were written. Use OPT_BYTE and OPT_LONG. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The egid variables are __u16 but parsed with OPT_UINT, which writes 32 bits and overwrites the next field. Use OPT_SHRT. Signed-off-by: Daniel Wagner <dwagner@suse.de>
lba-format-index and cntlid are stored in __u16 variables, but OPT_UINT writes 32 bits and overwrites the next field. Signed-off-by: Daniel Wagner <dwagner@suse.de>
domain-id and endg-id are __u16 variables parsed with OPT_UINT, which writes 32 bits and overwrites the next field. log-entries is an __u64 of which OPT_UINT writes only the first half, the upper half on big-endian machines; the count is limited by 32-bit and 16-bit identify fields, so make it an __u32. Signed-off-by: Daniel Wagner <dwagner@suse.de>
OPT_FLAG stores a bool; declare iekey and eds as bool instead of __u8 like the other reservation commands do. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Describe how tests are named, what the groups mean and how to select tests by group or by name. Signed-off-by: Daniel Wagner <dwagner@suse.de>
With only CC=clang, the C++ tests of libnvme are built and linked by the gcc C++ compiler, with the gcc sanitizer runtime. Where that is not installed, meson fails while generating the targets: ERROR: Linker cc does not support sanitizer arguments ['-fsanitize=address,undefined'] And mixing the runtimes of two toolchains does not work anyway. Fail at the start of the setup instead, and name the fix: CC=clang CXX=clang++. Signed-off-by: Daniel Wagner <dwagner@suse.de>
clang links the sanitizer runtime only into executables, so a shared library of a sanitizer build has undefined references to it, which -Wl,--no-undefined rejects: FAILED: libnvme/tests/libmock-ifaddrs.so undefined reference to `__asan_report_load8' meson only warns about b_lundef with a clang sanitizer build. Fail the setup with the option to set instead. Signed-off-by: Daniel Wagner <dwagner@suse.de>
-Dfuzzing=true adds -fsanitize=fuzzer-no-link to all C code. The fuzz targets get the coverage hooks from libFuzzer, every other program from the sanitizer runtime. Without a sanitizer the test programs fail to link: undefined reference to `__sanitizer_cov_trace_const_cmp4' Fail the setup instead. Signed-off-by: Daniel Wagner <dwagner@suse.de>
…builds build.sh -c clang -s sets only CC, so the build mixes the clang and gcc toolchains, and with clang the shared libraries need b_lundef=false. Set CXX to the clang++ next to the clang in CC, unless CXX is set, and pass -Db_lundef=false. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The libFuzzer setup misses CXX=clang++ and -Db_lundef=false, so it does not build. The device example runs on the argconfig corpus, and both examples write the inputs libFuzzer finds into the source tree; the comment in tests/fuzz/meson.build runs a target on the seeds in the tree directly. And allocator_may_return_null alone does not stop the device target from aborting: libFuzzer reports a device sized allocation as out of memory before ASan can fail it. Add the clang setup to the sanitizer section and use it for the libFuzzer build. Run each target on a copy of its own seeds in the build directory, cap the ASan allocation size for the device target, and say where a crashing input goes. Point the comment to the documentation. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The --e2e-* and --plugin-tests options are only valid with the tests config, which one condition over all their variables checks. Let every config specific option record its config instead, and check them all in one loop. Another config can then have options of its own, and the error names the option which does not fit. Signed-off-by: Daniel Wagner <dwagner@suse.de>
A long fuzzing run needs the clang sanitizer build, a copy of the seeds per target, the right options per target and a place for the results. Add a fuzz config which builds the targets for libFuzzer and runs each of them in turn: --fuzz-seconds n seconds per target (600) --fuzz-jobs n libFuzzer jobs per target (one per CPU) --fuzz-targets list the targets (all) --fuzz-dir dir the results (.build-ci/fuzz) The jobs run as independent processes on a shared corpus (-jobs). The newer -fork mode would do the same, but the libFuzzer of clang 22 crashes in it. The results are summary.txt with the findings, and per target the logs of the jobs, the inputs which found a bug and the corpus. A finding stops its job, a UBSan finding like a crash. The exit status is not zero if a target had a finding. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The --raw-binary flag only sets cfg.raw_binary when it is given, so without it query-cap reads an uninitialized bool. UBSan reports it while fuzzing the device data: sfx-nvme.c:731:12: runtime error: load of value 192, which is not a valid value for type 'bool' Initialize the options like the other commands do. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The stdout and the JSON printer walk the descriptors with the sizes the
device reports, without checking them against the length of the log.
A descriptor at the end of the log is read beyond it, and a size larger
than the rest of the log moves the walk out of it:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 8
#0 json_hwcomp_log plugins/ocp/ocp-print-json.c:131
Add hwcomp_desc_entry(), which returns a descriptor only if its header,
date/lot code and additional information fit into the rest of the log,
and walk the descriptors with it in both printers.
Signed-off-by: Daniel Wagner <dwagner@suse.de>
The Event Header Length counts the bytes of the event header after the
first 3, but the printers of the Persistent Event log read all header
fields of an event however short its header is. An event of a zeroed
log has only 3 bytes, so a log of 2 MiB is printed as some 700000
events, which takes minutes:
ALARM: working on the last Unit for 15 seconds
linux-nvme#4 shr_table_align_column shared/table-util.c:353
linux-nvme#5 stdout_kv_table_finish src/nvme-print-stdout.c:609
linux-nvme#6 nvme_show_pel_event_header src/nvme-print-stdout-log.c:244
The same walk is in five printers. Add nvme_pel_event(), which returns
an event only if its header holds all header fields, its vendor
specific information fits into the event and the event into the log,
and use it in all of them.
Signed-off-by: Daniel Wagner <dwagner@suse.de>
nvmf_create_discovery_ctrl() reaches the libnvme_open() call only after __create_discovery_ctrl() has already connected the controller and linked it into the subsystem tree. The identify-failure path right below handles this correctly by calling libnvmf_disconnect_ctrl() followed by libnvme_free_ctrl(). The libnvme_open() failure path, however, returns the error without tearing anything down, so the freshly created discovery connection is left established even though the operation failed. Disconnect and free the controller on this path too, matching the identify-failure path. Signed-off-by: Martin George <marting@netapp.com>
libnvme_ctrl_scan_namespaces() stores the return of libnvme_scan_ctrl_namespaces() into a signed count and loops over it without first checking for a negative error. On a scan failure the negative count simply skips the loop and the function returns success, silently hiding the error. Its siblings libnvme_ctrl_scan_paths() and nvme_subsystem_scan_namespaces() both bail out on a negative count. Check for the error and return it, matching the other scanners. Signed-off-by: Martin George <marting@netapp.com>
libnvme_transport_handle_set_submit_entry() assigns the caller-provided submit_entry hook, but the subsequent NULL check and default assignment operate on submit_exit instead of submit_entry (a copy-paste slip; the sibling set_submit_exit()/set_decide_retry() setters correctly guard their own field). As a result, passing NULL to reset the entry hook to the library default leaves hdl->submit_entry NULL rather than restoring __libnvme_submit_entry. The next command execution then calls hdl->submit_entry() and dereferences a NULL function pointer. Guard and default submit_entry itself. Signed-off-by: Martin George <marting@netapp.com>
save_discovery_log() computes the discovery log size (header plus numrec * sizeof(entry)) into an int before passing it to shr_write_all(), whose length argument is a size_t. numrec is a 64-bit field read from the discovery log header, so a sufficiently large device-controlled record count can overflow the int and truncate the length, causing a short or corrupted saved log. Compute the length as size_t so the full byte count is preserved. Signed-off-by: Martin George <marting@netapp.com>
fabrics_dim() overwrites ret on every dim_operation() call, so when several controllers or devices are addressed only the status of the last one is returned; an earlier failure is masked by a later success. Keep the first error in a separate variable and return that, so a failure on any controller is reported. Signed-off-by: Martin George <marting@netapp.com>
json_output_key_value() passed strdup(key) through trim_white_space() and stored the result in a __cleanup_free pointer. trim_white_space() skips leading whitespace by advancing the pointer and returning that advanced position. When the key has leading whitespace, key_trim points into the middle of the strdup() allocation rather than to its start, so the __cleanup_free at scope exit is called on a pointer that was not returned by malloc()/strdup(), leading to undefined behavior and potential heap corruption. Keep the original allocation in its own __cleanup_free variable and let key_trim be a mere view into it, so the correct base pointer is freed. Signed-off-by: Martin George <marting@netapp.com>
stdout_feature_show_fields() stores the decoded fields in a __u8, but TMPTH, TMT1, TMT2 and IIELL are 16 bits wide. Only the low byte is printed, so a temperature threshold of 343 K is shown as 87 K, and an IIELL of 30000us as 4400us. The JSON output is correct. Fixes: cdce9c0 ("libnvme, nvme: add Idle I/O Exit Latency Limit (TP4204)") Fixes: 20b0e89 ("nvme-print-stdout: convert stdout_feature_show_fields() to shr_table") Signed-off-by: Iain <iain1970@protonmail.com>
The decoded timestamp value is incorrect because bytes 6 and 7 of the timestamp field used for timestamp attributes are not masked off. test: cover OCP telemetry timestamp attributes Added test with non-zero values in timestamp bytes 6 and 7 to verify change. Signed-off-by: Jeff Lien <jeff.lien@sandisk.com> Reviewed-by: Brandon Paupore <brandon.paupore@sandisk.com>
jeff-lien-sndk
force-pushed
the
timestamp-decode-fix
branch
from
October 19, 2026 18:50
8395804 to
96e258b
Compare
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
json_ana_log() creates the JSON object of a group descriptor before it
checks that the namespace ID list fits into the log, and leaks it when
the check stops the loop:
SUMMARY: AddressSanitizer: 744 byte(s) leaked in 3 allocation(s).
#2 json_object_new_object
linux-nvme#3 json_ana_log nvme-print-json.c
Create the object once the descriptor has passed the check.
Fixes: 4b5892f ("nvme-print: fix out-of-bounds reads in ANA log parsers")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
json_print_smart_log() creates the JSON object of an extended SMART
attribute for every slot of the log, but adds it to the output only
when the slot holds an attribute. The objects of the empty slots leak:
SUMMARY: AddressSanitizer: 31248 byte(s) leaked in 126 allocation(s).
#2 json_object_new_object
linux-nvme#3 json_print_smart_log seagate-nvme.c:466
This is also why the seagate vs-smart-add-log test fails in sanitizer
builds. Create the object only for a slot which holds an attribute.
Fixes: ec9837b ("Initial checkin for Seagate Plugin")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
get_market_log() prints the 512 bytes of the marketing name log with
%s. A log without a NUL character makes printf() read past the buffer
on the stack:
SUMMARY: AddressSanitizer: stack-buffer-overflow
#2 printf
linux-nvme#3 get_market_log intel-nvme.c:423
Bound the string by the size of the log.
Fixes: bae7966 ("Add intel marketing log page")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
amzn_id_ctrl() copies the block device name from the vendor specific
area of Identify Controller with snprintf(), which limits what it
writes, but reads the name as a string: without a NUL character in the
rest of the area, it reads past the end of the Identify data.
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1025
#2 amzn_id_ctrl amzn-nvme.c:122
Read no more than the name.
Fixes: fc8f734 ("id-ctrl vs for Amazon Elastic Block Store")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
sldgm_get_market_log() prints the 512 bytes of the marketing name log
with %s. A log without a NUL character makes printf() read past the
buffer on the stack:
ERROR: AddressSanitizer: stack-buffer-overflow
READ of size 513
#2 sldgm_get_market_log solidigm-market-log.c:63
Bound the string by the size of the log, as for the Intel log.
Fixes: fa57395 ("plugins/solidigm: Added Marketing Name Log support for Solidigm.")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
intel_id_ctrl() copies the health, bl, mic_bl and mic_fw fields of the
vendor specific area with %s. A field without a NUL character makes
snprintf() read on, past the end of the Identify Controller data:
SUMMARY: AddressSanitizer: heap-buffer-overflow
#1 snprintf
#2 intel_id_ctrl intel-nvme.c:110
Bound each string by the size of its field.
Fixes: 74d5070 ("Fixing Empty Fields of Intel VU id-ctrl")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
print_smart_cloud_health_log() adds the array of the log to the JSON
output only for the M51CX, M51BY and M51CY models, but the caller also
passes M6003 and M6004. For them the array and the object of the log
statistics leak:
SUMMARY: AddressSanitizer: 1080 byte(s) leaked in 6 allocation(s).
#2 json_object_new_array
linux-nvme#3 print_smart_cloud_health_log micron-nvme.c:1532
linux-nvme#4 micron_ocp_smart_health_logs micron-nvme.c:3416
Create the array only for the models with a layout of the log, and
free the statistics object when there is none. The output stays the
same: the plugin knows no layout of the log for M6003 and M6004.
Fixes: c298ea0 ("plugins/micron: update to support latest products")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 23, 2026
vt_parse_detail_identify() prints the serial number and the model
number of the Identify Controller data with %s. They are space padded
fields, not C strings: a device which fills them makes printf() read
past the data on the stack:
SUMMARY: AddressSanitizer: stack-buffer-overflow
#1 printf
#2 vt_parse_detail_identify virtium-nvme.c:720
Bound them by their size, like the firmware revision below.
Fixes: fb7f63b ("Add Virtium plugin extension")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
Author
|
This change was merged into the master branch of the main nvme cli repo with the PR #4226. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The decoded timestamp value is incorrect because bytes 6 and 7 of the timestamp field used for timestamp attributes are not masked off.