libnvme: probe 64-bit ioctl support without submitting a command#3349
Closed
Mateusz-Nowicki-Embedded wants to merge 1 commit into
Closed
libnvme: probe 64-bit ioctl support without submitting a command#3349Mateusz-Nowicki-Embedded wants to merge 1 commit into
Mateusz-Nowicki-Embedded wants to merge 1 commit into
Conversation
The ioctl_probing path in __nvme_transport_handle_open_direct() submits a zero-initialised passthru command via LIBNVME_IOCTL_ADMIN64_CMD just to detect whether the kernel recognises the ioctl number. The dummy reaches the admin queue, the controller rejects it with Invalid Queue Identifier, and the return value is used to decide whether to enable the 64-bit variants. Every controller open therefore submits a real, intentionally malformed admin command that the device actually fetches and completes. It shows up in traces, polluting the trace history with events that have nothing to do with what the caller asked for. That makes the path harder to reason about while debugging. Pass NULL instead. The kernel returns -ENOTTY when the ioctl number is unknown and -EFAULT (from copy_from_user(NULL)) when it is known, so the distinction lives entirely in errno and no NVMe command is ever submitted from the probe. Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
Collaborator
|
Thanks for the fix. Ideally we don't probe at all anymore, instead we just try the CMD64 variant when we issue the first command and if this fails we use the CMD32 version. Though this is slightly difficult to implement, it would make the overhead of probing going away for modern system, which is a good thing IMO. |
Collaborator
|
I've merged the defer/lazy ioctl approach. Hopefully, this should address the issue and make it actually work also for IO. Let me know if this doesn't work as expected. Thanks! |
Author
yep, I verified it and dummy cmd is not generated anymore, 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.
Replace the dummy admin SQE submission used to probe
NVME_IOCTL_ADMIN64_CMDavailability with a NULL-argp ioctl,which distinguishes "ioctl unknown" (ENOTTY) from "ioctl known, arg invalid" (EFAULT) without sending any
NVMe command to the controller.
Side benefits:
nsinfo leak through dummy.nsidDelete I/O SQSQE per controller open, which would otherwise be observable in admin SQ ring memoryand as an
Invalid Queue IdentifierCQE in tracesCompatible with all kernels supported by nvme-cli (>= v4.15):
NVME_IOCTL_ADMIN64_CMDdoesn't exist → ENOTTY, same outcome as todaycopy_from_user(NULL)fails with EFAULT before the command leaves nvme_user_cmd64()