Repository navigation
tests: raise CLI coverage with a print test on the mock device - #4151
Merged
Merged
Conversation
added 4 commits
October 9, 2026 14:14
The plugins need vendor hardware and hold more than half of all lines. With one total, progress in the CLI, libnvme and the daemons is hard to see. Report a separate number for each part of the tree. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Bad data from the mock device can make nvme allocate memory without bound or loop forever. One run grew to 26 GB and the kernel OOM killer stopped it and the terminal it ran in. Limit each run to 1 GB of memory and 60 seconds. With ASan, use hard_rss_limit_mb, because ASan reserves too much address space for RLIMIT_AS. Cross builds run nvme under qemu-user, which needs the whole guest address space, so they get no memory limit. Decode the output with replacement characters, because nvme prints the raw bytes of string fields. 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 fixed-size arrays, but each descriptor has a variable length. The counts in the log were not checked, so bad data made nvme read out of bounds and allocate gigabytes of memory. The log page has no length field. Read it again with a larger buffer until it holds all the descriptors, up to 256 KiB. Pass the length to the printers and stop at the end of the buffer. A Media Unit Descriptor Length that is odd leaves the next descriptors unaligned. The current specification requires this field to be 0, so copy each descriptor before reading it, in case a device or a future specification uses an odd value. Also fix the NVM Set index in the stdout output and the media unit keys in the JSON output (muid and mudl, not chanid and chmus). Signed-off-by: Martin Belanger <martin.belanger@dell.com>
The JSON printers read sysfs attributes with a NULL default and pass the result to obj_add_str(), which uses it as a format string. When the attribute does not exist, nvme crashes. This happens on real systems: ana_state is missing for PCIe multipath without ANA, iopolicy without native multipath, and phy_slot for controllers that are not PCIe. "nvme list -v -o json" and "nvme show-topology -o json" crash on such systems. Use an empty string as the default, as the stdout printers do. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
martin-belanger
force-pushed
the
coverage-raise
branch
from
October 9, 2026 20:48
5c2703a to
17faf2b
Compare
Most of the print code is not covered by tests. Run the id, log, feat and get-feature commands, and the read-only fdp, zns, resv and dir commands, on the mock device in the normal, verbose, JSON and binary formats. The mock device returns pseudo-random data from a fixed seed, so the print code takes many branches. Each command must exit without a crash, and the JSON output must parse. Also check the supported capacity configuration list log and the persistent event log with valid data, and run the list and topology commands on sysfs trees captured from real systems. The test does not run on big-endian hosts yet. It times out on s390x. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
martin-belanger
force-pushed
the
coverage-raise
branch
from
October 9, 2026 21:00
17faf2b to
ae32ce1
Compare
Collaborator
|
Very nice. Thanks! |
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
2 participants
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.
tests: raise CLI coverage with a print test on the mock device
The goal of this PR is to raise test coverage of the nvme CLI. The new test found several bugs. This PR fixes the ones the test needs. The others are listed below and will be fixed in follow-up PRs. More coverage work will also follow.
Coverage
print-cli. It runs the read-only id, log, feat, get-feature, fdp, zns, resv and dir commands on the mock device, in the normal, verbose, JSON and binary formats. The mock device returns pseudo-random data from a fixed seed, so the print code takes many branches. It also runs list, list-subsys and show-topology on the sysfs trees captured from real systems in libnvme/tests/sysfs.Local
meson test: CLI 18% -> 58%, total 29% -> 42%.Bugs fixed
nvme log supported-cap-configread only the 16-byte log header, then printed the descriptors from memory past the buffer. It indexed variable-length descriptors as fixed-size arrays, and did not check the counts. With bad data it allocated more than 20 GB.nvme list -v -o json,nvme list-subsys -v -o jsonandnvme show-topology -o jsoncrashed when a sysfs attribute was missing: ana_state (PCIe multipath without ANA), iopolicy (no native multipath), phy_slot (not PCIe).Bugs found, not fixed here
Real devices:
nvme log persistent-eventnever prints the last event. The bounds check rejects an entry that ends exactly at the end of the log.nvme log media-unit-statreads only the log header, then prints the descriptors from memory past the buffer.obj_add_str()passes the value as a printf format string. A device string that contains '%' is interpreted.Big-endian:
print-clitimes out on s390x (more than 300 s; about 55 s on armhf and ppc64le). Some command probably reads a little-endian field without conversion and then loops or reads for a long time. Not found yet, because the CI log of the s390x job is not kept. The test runs only on little-endian hosts until this is found.Only with counts that the specification does not allow (hardening):
id ctrl,id ns,id nvmset,id domainandid ns-granularityprinters.fdp eventsandzns changed-zone-list.get-feature -f 0x1euses the whole result dword as the event count, not the low byte.