libnnvme, nvme-cli: Discover rewrite - #3828
Open
martin-belanger wants to merge 9 commits into
Open
Conversation
added 9 commits
August 12, 2026 06:51
_nvmf_discover() and nbft_discovery()/nbft_connect() clone the context per log entry, then pass that clone to hooks and to the next recursion level. The clone shallow-copies hostnqn, hostid, tls_key, and nbft_path, so a hook freeing one via a public setter would corrupt the real context. Pass the real context to hooks and recursion instead. Also move set_discovery_kato() back to before the connect, restricted to discovery-type entries: referral controllers were connecting without a keep-alive timeout, only reaching grandchildren through the clone's inheritance chain. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
discover_lookup_ctrl_by_device() and discover_lookup_ctrl() still wrote fctx->persistent = NO on their two fallback paths (device not found, not a discovery ctrl) -- the same bug class already fixed for the FORCE overrides, just missed since these aren't FORCE. Merge both into dc_open(), which never touches fctx->persistent and reports DC_OWNED/DC_BORROWED instead of the already_connected bool. nvmf_create_discovery_ctrl() stays separate since libnvmf_discover_nbft() also calls it directly. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
--force used to do double duty: force persistence, and skip reusing an existing connection. Now that --persistent=force exists as its own explicit option, --force only ever means the latter -- so the name no longer matches what it does. Add --no-reuse as the real name; keep --force as a deprecated alias so existing scripts and discovery.conf files keep working. Rename the backing fctx->force field and its accessors to match, and check_ctrl_owner()'s force parameter, which shares the same concept. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
_nvmf_discover() found c's own discovery log entry by checking whether lookup_ctrl() resolved back to c. Simpler and more direct to just check the entry's SUBTYPE for NVME_NQN_CURR (03h), which is exactly what that field is for. Self entries never reach nvmf_connect_disc_entry() or dc_should_connect() now, so the SUBTYPE 03h case and the DUPRETINFO check in those two functions were dead code, removed. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
_nvmf_discover() skipped an already-connected referral outright: no DLP fetch, so anything only reachable through it was silently dropped. Its only cycle protection was an incidental side effect of looking up live connections, not an explicit guard. Split into two passes per DC: find and act on the self entry (SUBTYPE 03h) first, since it is the only place a DC's own EPCSD is reported, then walk referrals and NVM subsystem entries. Add a whole-walk TID-keyed visited set (dc_visited) so a DC is walked once regardless of how many paths reach it, and an 8-level depth cap per spec. The visited set also decouples dedup from whether a connection is still open, which is what makes it safe to disconnect a DC right after its own DLP fetch instead of waiting for the whole walk to finish -- the old code could not do this, since a torn-down DC disappeared from the live connection lookup it relied on for dedup. Extract the disconnect decision into dc_decide(), a pure function of a DC's own facts and policy, with unit tests (test_dc_decide()) -- the first tests this code has ever had. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
nbft_discovery() was a forked copy of _nvmf_discover()'s per-DC walk, missing the depth cap and visited-set the general path just gained. Replace it with dc_open()/dc_walk(), the same machinery discover and connect-all use, via a new private connect_leaf hook so NBFT's leaf-connect quirks (DHCP retry, firing hooks.connected) stay NBFT-only without forking the walk itself. NBFT never honors --no-reuse (dc_open() gains honor_no_reuse) and never respects --persistent (fctx->persistent forced to NO before the walk): boot discovery is a one-shot operation where reusing an existing connection is strictly better, and neither flag was ever consulted by the old code either. Two real, pre-existing bugs found and fixed along the way: host_iface was freed before nbft_discovery()'s walk read it from every per-entry connect during that DC's DLP walk; and the Discovery Descriptor connection's own DHCP retry cleared traddr (the destination) instead of host_traddr (the local address), which cannot ever succeed. Also added lookup_live_ctrl(), deduplicating a lookup_ctrl() + name check repeated at four call sites, one of them new here. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
_nvmf_discover()'s NVME_NQN_NVME and NVME_NQN_DISC branches connect new controllers via nvmf_connect_disc_entry() directly, without ever calling fctx->hooks.connected(). The hook is registered by every discover/connect-all invocation path (src/fabrics.c's hook_connected, which prints or JSON-emits per-device connect output), and already fires correctly for a direct "nvme connect" and for NBFT's own leaf-connect path -- just not here, so connect-all has never reported per-device progress for anything it discovers and connects itself. Fire it after a successful connect in both branches, guarded to skip the NVME_NQN_NVME case when a connect_leaf hook is set: NBFT's own hook already fires hooks.connected internally, and firing it again here would double it. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
read_discovery() treated an absent DCNQNHOR (Discovery Controller NQN Heap Object Reference) as a parse failure, dropping the whole Discovery Descriptor silently. Per the NVMe Boot Specification, a DCNQNHOR cleared to 0h just means "no unique NQN, use the well-known one" -- the common case, not a malformed table. Tolerate -ENOENT there specifically; every other error from the same heap-object lookup still fails the descriptor as before. Even for a Discovery Descriptor with a real DCNQNHOR, libnvmf_discover_nbft() never read the parsed struct libnvmf_discovery.nqn, always connecting with the well-known NQN instead. Use it when present. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
libnvmf_connect() and libnvme_add_ctrl() called fctx->hooks.connected, already_connected, and decide_retry directly, with no NULL check. libnvmf_context_create() accepts NULL for any of the three with no validation, and every other call site in this file that invokes the same hooks (dc_already_connected(), nbft_connect(), the connect_leaf path) already guards them. libnvmf_connect() is a public API function, so any external caller passing NULL for a hook it doesn't need -- a normal, otherwise-supported pattern -- would crash the first time that code path fires. decide_retry's guard defaults to "don't retry" when unset, via the same short-circuit && the other two use as an unconditional guard. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Author
|
Looks like the windows build is timing out |
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
1 participant
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.
Rewrites libnvme's Discovery Controller walk (
_nvmf_discover(),libnvmf_discover(),libnvmf_discover_nbft()) and its supporting connection-resolution code infabrics.c. This backsnvme discover,nvme connect-all, and NBFT boot discovery.Four main things in here:
nvme discover/connect-alluse, instead of a second, forked implementation of the same walk.--forcerenamed to--no-reuse(kept as a deprecated alias):--force's original job was working around a persistence bug that's since been fixed elsewhere, so what's left is only its narrower meaning, "never reuse an existing connection." This is also the one commit that touches more than one file -- every other commit is mostly justfabrics.c.Bugs found and fixed along the way (mostly NBFT related), none part of the original plan, all found while working through the surrounding code:
_nvmf_discover()never firedhooks.connectedfor anything it discovered and connected itself, referrals or IOC entries --connect-allhas never reported per-device progress for its own discovery-driven connects, even though the hook exists and fires correctly everywhere else.fctx->hooks.*calls (connected,already_connected,decide_retryinlibnvmf_connect()) had no NULL check, even though the hooks are documented-optional and every other call site in the file guards them. Any external libnvme caller passing NULL for a hook it doesn't need would crash.host_ifacewas freed before its own walk started, then read (use-after-free) by every connect made during that walk.read_discovery()(nbft.c) treated an absent DC NQN reference as a parse failure, silently dropping the whole Discovery Descriptor. Per the Boot Specification, absent just means "use the well-known NQN" -- the common case, not a malformed table.libnvmf_discover_nbft()still never read the parsed NQN when present, always connecting with the well-known one instead.Commits, in order. Commit 5 is the substantial one -- the actual rewrite; the rest are mostly small and mechanical:
62779e721-- stop handing per-entry context clones to hooks/recursion (a real unsoundness: the clone shallow-aliases owned pointers a hook could free via a public setter)0f5e6d8a4-- unify the four resolve-or-create-a-DC paths into onedc_open()a1150d09d-- rename--forceto--no-reuse(kept as a deprecated alias); the old name never described what it did02c75ab01-- identify a discovery self entry by its spec-defined SUBTYPE instead of a pointer-identity coincidence32e6c0bae-- the core rewrite: depth-first walk with a visited-set and depth cap,dc_decide()extraction, and its first unit testsac3caf4b6-- fold NBFT's forked walk onto the same machinery (UAF and dead-retry fixes included)56b9f0221-- firehooks.connectedfor discovery-walk connects663dfcca5-- NBFT Discovery Descriptor NQN parsing and consumption fixes9a703c7f5-- guard the three unchecked hook callsVerification: every commit builds and tests standalone;
-Dwerror=trueand-Dfabrics=disabledboth clean throughout; full unit suite green, including the 9 newdc_decide()cases. Live-smoke-testednvme discoveragainst a real TCP discovery controller. Not tested against a real NBFT boot environment.