Caleb Sander [Thu, 30 Nov 2023 18:58:22 +0000 (11:58 -0700)]
tree: fix incorrect return value
nvme_scan_subsystem() accidentally sets errno = -EINVAL
if the matching subsystem is filtered out.
This errno value will be overwritten by the out_free path with -ret,
where ret is the length of the path string.
Set ret = -EINVAL instead, so errno will be set to EINVAL.
This matches the behavior in the case where a new subsystem is allocated
and fails the filter function.
Hannes Reinecke [Thu, 16 Nov 2023 06:41:37 +0000 (07:41 +0100)]
libnvme: support NVMe TLS identities version 1
With NVMe TP8018 a new version '1' for generating NVMe TLS identities
was specified; identities generated for this version require a PSK hash
to be attached to the version '0' identifier.
This patch implements a new function 'nvme_insert_tls_keys_versioned()'
to support this functionality and makes the original function
'nvme_insert_tls_keys()' a wrapper for the new function.
Daniel Wagner [Thu, 16 Nov 2023 14:28:25 +0000 (15:28 +0100)]
build: do not rebuild muon/samu every time
There is not need to build the build tools everytime. Thus stage the
samurai and muon build into a new top level directory .build-tools and
only build them on demand.
Caleb Sander [Tue, 8 Aug 2023 01:21:12 +0000 (19:21 -0600)]
fabrics: avoid redundant args in nvme_discovery_log()
nvme_discovery_log() takes nvme_get_log_args as input
and then overwrites many of its fields.
This leads to wasted code in the caller setting up unused fields.
Instead pass nvme_get_discovery_args, which more accurately
expresses the possible discovery log arguments.
As an added benefit, it also subsumes the other arguments.
And nvmf_get_discovery_wargs() can pass its arguments straight through.
Caleb Sander [Tue, 8 Aug 2023 00:59:10 +0000 (18:59 -0600)]
fabrics: have nvmf_get_discovery_log() call nvmf_get_discovery_wargs()
There is a lot of duplicated code between nvmf_get_discovery_log()
and nvmf_get_discovery_wargs().
Since nvmf_get_discovery_wargs() is more general,
use it to implement nvmf_get_discovery_log().
Caleb Sander [Tue, 1 Aug 2023 17:24:15 +0000 (11:24 -0600)]
fabrics: fetch smaller Discovery Log Page header
Most of the 1 KB Discovery Log Page header is reserved.
Only the first 18 bytes are currently defined.
So avoid transfering more data from the controller than necessary.
Caleb Sander [Tue, 1 Aug 2023 01:57:54 +0000 (19:57 -0600)]
fabrics: avoid redundant Get Log Page on retry
In case of a GENCTR mismatch, nvme_discovery_log() currently fetches
the Discovery Log Page header again before fetching the entries.
This is unnecessary since it was just fetched to determine GENCTR.
So only fetch the header once and re-use it for the next loop iteration.
Remove a couple of unnecessary variables in the process.
Caleb Sander [Tue, 1 Aug 2023 01:29:03 +0000 (19:29 -0600)]
fabrics: clear RAE for discovery log page commands
In several circumstances, nvme_discovery_log() leaves the RAE bit set
for all Get Log Page commands it issues.
This happens if the header indicates the log page has no records,
or if a command other than the header re-fetch errors out.
If the RAE bit is never cleared, the controller will not send more
Discovery Asynchronous Event Notifications.
Setting the RAE bit is not necessary to avoid missed events since
the use of GENCTR ensures the log page is fetched atomically.
It is better to risk receiving additional AENs
than for the discovery controller to never send more AENs
because it is waiting for a Get Log Page command that never comes.
So clear the RAE bit when fetching each piece of the log page.
Caleb Sander [Thu, 2 Nov 2023 13:29:56 +0000 (14:29 +0100)]
test: Cast values to u32 if shift overflows int
Bit shifts that overflow the resulting type are undefined behavior in C.
C arithmetic promotes to ints all smaller integer types.
There are several places where a 32-bit unsigned value
is constructed by shifting a u8 or u16 to the most significant bits.
Since this overflows a signed 32-bit integer,
explicitly cast to u32 to avoid the UB.
Technically, an int is allowed to only be 16 bits,
so any shift that could set bit 15 or higher is UB.
But platforms where int is s16 are not very common,
so it's likely not worth the effort to fix the code.
Caleb Sander [Thu, 2 Nov 2023 13:29:46 +0000 (14:29 +0100)]
mi: Cast values to u32 if shift overflows int
Bit shifts that overflow the resulting type are undefined behavior in C.
C arithmetic promotes to ints all smaller integer types.
There are several places where a 32-bit unsigned value
is constructed by shifting a u8 or u16 to the most significant bits.
Since this overflows a signed 32-bit integer,
explicitly cast to u32 to avoid the UB.
Technically, an int is allowed to only be 16 bits,
so any shift that could set bit 15 or higher is UB.
But platforms where int is s16 are not very common,
so it's likely not worth the effort to fix the code.
Caleb Sander [Sat, 14 Oct 2023 01:28:33 +0000 (19:28 -0600)]
types: Cast values to u32 if shift overflows int
Bit shifts that overflow the resulting type are undefined behavior in C.
C arithmetic promotes to ints all smaller integer types.
There are several places where a 32-bit unsigned value
is constructed by shifting a u8 or u16 to the most significant bits.
Since this overflows a signed 32-bit integer,
explicitly cast to u32 to avoid the UB.
Technically, an int is allowed to only be 16 bits,
so any shift that could set bit 15 or higher is UB.
But platforms where int is s16 are not very common,
so it's likely not worth the effort to fix the code.
Caleb Sander [Thu, 2 Nov 2023 13:26:36 +0000 (14:26 +0100)]
test: Avoid unaligned pointer dereferences
Avoid casting byte-aligned pointers to pointers with higher alignment.
Loading or storing values with higher alignment is undefined behavior,
since some processors don't allow unaligned memory accesses
and compilers may assume pointers of different types don't alias.
Perform an explicit memcpy(), which an optimizing compiler
can easily replace with a single load/store on supported architectures.
Caleb Sander [Sat, 14 Oct 2023 04:35:43 +0000 (22:35 -0600)]
nbft: Avoid unaligned pointer dereferences
Avoid casting byte-aligned pointers to pointers with higher alignment.
Loading or storing values with higher alignment is undefined behavior,
since some processors don't allow unaligned memory accesses
and compilers may assume pointers of different types don't alias.
Perform an explicit memcpy() in two places, which an optimizing compiler
can easily replace with a single load/store on supported architectures.
While we're touching this code, also use IN6_IS_ADDR_V4MAPPED()
instead of hand-rolling it.
Caleb Sander [Sat, 14 Oct 2023 01:28:09 +0000 (19:28 -0600)]
test: don't pass NULL to memcmp() or memset()
According to the C standard, it is undefined behavior
to pass NULL pointers to standard library functions.
This includes the mem*() family of functions,
even they are passed a length of 0.
So avoid calling these functions when the length is 0.
Caleb Sander [Sat, 14 Oct 2023 01:13:56 +0000 (19:13 -0600)]
test: make LD_PRELOAD tests work with ASAN
Several tests mock libc functions by setting LD_PRELOAD
to a shared library containing mock implementations of the functions.
Currently this prevents the tests from running with ASAN.
ASAN complains that libasan doesn't come first in LD_PRELOAD and aborts.
But in practice this doesn't seem to be an issue.
Sample ASAN issues added to libnvme, the test cases, and the mocks
are all correctly reported.
So suppress this warning by setting the environment variable
ASAN_OPTIONS=verify_asan_link_order=0.
In case the user does want to inject a specific libasan,
change the tests' use of LD_PRELOAD to append the mock shared library
rather than overwriting LD_PRELOAD entirely.
Caleb Sander [Fri, 13 Oct 2023 23:41:30 +0000 (17:41 -0600)]
readme: clarify that LD_PRELOAD is not needed for ASAN
The README currently suggests setting LD_PRELOAD to the libasan path.
However, this seems to be unnecessary (at least on my gcc 9 setup).
The test executables request to link with libasan.so (before libc.so),
so it is automatically loaded.
The recommended suggestion also ends up running ninja with ASAN
instrumentation, which causes it to fail with leaks detected.
Joy Gu [Wed, 25 Oct 2023 20:47:42 +0000 (13:47 -0700)]
types: add cross-namespace copy formats, status codes, ONCS bits
Add support for NVMe TP4130 ("Cross-Namespace Copy"):
- Add Copy Descriptor Formats 2h and 3h
- Add new status codes for Copy: Incompatible Namespace or Format, Fast
Copy Not Possible, Overlapping I/O Range, and Insufficient Resources
- Add two new ONCS bits NVMCSA and NVMAFC
- Add Copy Descriptor Formats Enable (CDFE) to Host Behavior Support
Data Structure
[dwagner: whitespace cleanups
moved new functions to 1.7 linker section] Signed-off-by: Daniel Wagner <dwagner@suse.de>
Caleb Sander [Tue, 20 Jun 2023 03:17:16 +0000 (21:17 -0600)]
fabrics: use SECTYPE to determine whether to use TLS
The NVMe specfications are clear that the discovery log page entry
SECTYPE field indicates whether TLS is supported.
Currently the TREQ field is used, which results in enabling TLS
even when SECTYPE = "No Security" and TREQ = "Not required".
Only enable TLS if SECTYPE indicates a TLS version is enabled.
From the NVMe/TCP transport specification, version 1.0c:
Security Type (SECTYPE): Specifies the type of security used by the
NVMe/TCP port. If SECTYPE is a value of 0h (No Security), then the host
shall set up a normal TCP connection.
From TP 8018:
The SECTYPE value of 0h (i.e., No Security) specifies that TLS is not
supported.
From TP 8025 (the "TLS Permitted" host case):
If the SECTYPE field in the TSAS field in the discovery log entry for
the remote entity is cleared to zero and the TSC field is not set to 01b
(i.e., Required), then initiate TCP connections without TLS.
Brandon Paupore [Mon, 9 Oct 2023 21:19:17 +0000 (16:19 -0500)]
ioctl: MSB variable-size storage/reference tags
The spec defines these values on a bitwise basis within the shared
80-bit region, stored from MSB to LSB for each tag. Mechanically this
can be achieved by setting the values as big-endian using the same logic
currently in place.
Sam James [Sat, 30 Sep 2023 05:38:53 +0000 (06:38 +0100)]
test: handle POSIX ioctl prototype
glibc has the following prototype for ioctl: int ioctl(int fd, unsigned long request, ...)
POSIX (inc. musl) has the following for ioctl: int ioctl(int fd, int request, ...)
Check which prototype is used in <sys/ioctl.h> to avoid a conflict and conditionally
define the right one for the system.
Bug: https://bugs.gentoo.org/914921 Signed-off-by: Sam James <sam@gentoo.org>
Jeremy Kerr [Sat, 23 Sep 2023 16:44:50 +0000 (09:44 -0700)]
mi-mctp: Fix free() in error path of mi_open_mctp
If the malloc of our struct nvme_mi_transport_mctp fails, we'll attempt
to free ->rsp_buf of this (now zero) pointer.
Instead, structure the error path to progressively undo the
initialisation operations. This means we'll need to save the errno at
the site of each possible failure.
In doing this, add a comment to the call to nvme_mi_close(), just to
clarify behaviour with regards to the cleanups through that path.
Reported-by: Barnabás Pőcze <pobrn@protonmail.com> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
This implements support for TP4119a, adding various fields and functions
to enable better handling of the new Physical Receiver Eye Opening
Measurement log page.
ioctl: pass NSID in Get/Set Features commands that use it
Several features are configured on a per-namespace basis
by setting the NSID in the Set Features and Get Features commands.
But the corresponding nvme_{g,s}et_features_*() functions
aren't passing the NSID in the commands.
For the functions missing a NSID parameter, define new variants
with the NSID argument added and mark the old functions as deprecated.
ioctl: fix swapped parameters in nvme_set_features_host_id()
nvme_set_features_host_id()'s prototype says the parameter EXHID
comes before SAVE, but its definition does the opposite,
causing the parameters to be misinterpreted.
Match the definition to the declaration.
nvme_get_features_iocs_profile() and nvme_set_features_iocs_profile()
are defined but not exorted in libnvme.so.
nvme_set_features_iocs_profile()'s prototype was also removed,
so add it back.
IOCSI is a 9-bit value, so fix its bitmask and change its type to u16.
According to the NVMe spec, the Host Behavior
and Write Protection features are not saveable.
Setting the SAVE bit may cause the Set Features command to be rejected,
so don't set it for these features.
The last 2 bytes of the struct nvme_timestamp data
are not initialized in nvme_set_features_timestamp().
Add an initializer to avoid sending uninitialized stack memory
to the controller.
Several nvme_{g,s}et_features_*() functions take a data buffer
but don't provide it to the Get/Set Features commands.
Fix them to pass the data buffer to nvme_{g,s}et_features().
Getting the Host Memory Buffer feature also returns
a struct nvme_host_mem_buf_attrs data structure,
so make a nvme_get_features_host_mem_buf2() function
that takes in a data buffer to receive it.
Martin Belanger [Tue, 12 Sep 2023 19:55:55 +0000 (15:55 -0400)]
tree: Add 2 new public functions to lookup existing controllers
There is duplicate code between libnvme and nvme-cli. When trying
to instantiate a controller, both libnvme and nvme-cli scan the
sysfs looking for an existing controller that can be reused.
This patch adds nvme_ctrl_find() and nvme_ctrl_config_match().
These functions can be used by nvme-cli to lookup existing
controllers. This will allow removing the duplicate code in
nvme-cli (separate commit to follow on the nvme-cli GitHub repo).
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
da Cunha, Leonardo [Wed, 23 Aug 2023 18:26:39 +0000 (11:26 -0700)]
linux: Added functions to enable faster telemetry data retrieval.
Moved telemetry data area support detection into separate function.
Added possibility to modify data transfer chunk size.
Enable telemetry extraction up to specified data area.
Removed some printf() and perror().
Caleb Sander [Wed, 30 Aug 2023 16:45:57 +0000 (10:45 -0600)]
test: account for discovery log page entry stripping
The discovery log page tests incorrectly assumed the log page
nvmf_get_discovery_log() returns exactly matches what was fetched.
This failed to account for the trailing space trimming
of the TRSVCID and TRADDR fields.
If the arbitrary entries happened to contain trailing spaces
for these fields, the test cases would fail.
Generate arbitrary entries with space-padded strings for the log page
and corresponding expected entries with NUL-padded strings.
The NVMe base specification defines fields TRSVCID and TRADDR
as "ASCII strings", meaning they are space-padded.
Therefore, whether spaces need to be stripped from them
doesn't depend on the transport or address family.
Some combinations already appear to be missing,
such as RDMA using IB addresses.
And if new transports are added in the future,
sanitize_discovery_log_entry() would need to be updated.
So strip spaces from these fields regardless of the entry's
transport type or address family.
Caleb Sander [Wed, 30 Aug 2023 15:44:48 +0000 (09:44 -0600)]
fabrics: only look for spaces in strchomp()
strchomp() is only needed to strip trailing spaces from a string.
There is no need to replace NUL characters with NUL characters.
So simplify the check for "NUL or space" to just "space".
Update the function description as well,
as it doesn't strip all whitespace, just spaces.
Daniel Wagner [Thu, 31 Aug 2023 14:19:50 +0000 (16:19 +0200)]
build: use latest container instead fixed version
We control the build containers so there is little risk
that these randomly break. So let's go with the latest
version and avoid updating the build files all the time.
Martin Belanger [Fri, 18 Aug 2023 01:35:34 +0000 (21:35 -0400)]
tree: Improve TCP controller matching algorithm
The configuration parameters used to connect to a TCP controller are:
1. transport - "tcp"
2. traddr - Destination address (controller's IP address)
3. trsvcid - Service ID (controller's port - typically 8009 or 4420)
4. host-traddr - Source address (host's IP address)
5. host-iface - Physical/Logical interface where the connection will be made
For TCP, transport, traddr, and trsvcid are mandatory, while the
host-traddr and host-iface are optional. The host-traddr and host-iface
can be used as "overrides" to select a different source address and
interface than those that the kernel would choose by default.
When an application using libnvme to connect to a controller does
not specify the host-traddr or host-iface, the kernel will have to
determine the best interface and source address by itself. It does that
by looking up the destination address (traddr) in the routing table to
determine the best interface for the connection. The kernel then
retrieves the primary IP address assigned to that interface and uses that
as the connection's source address. By default, the kernel always uses
the interface's primary address as the connection's source address
unless host-traddr is used to override it.
Prior to version 6.1, the kernel did not reveal the source address or
interface it selected. Therefore, it was impossible for user-space apps
to tell exactly where connections were made. With kernel 6.1 (and later),
the sysfs now exposes the source address as "src_addr=" in the nvme
"address" attribute. The src_addr not only provides us with the
connection's source address, but by scanning the interface map one can
find out which interface owns that source address and precisely determine
on which interface each connection is made.
With TP8010 and the introduction of the Centralized Discovery Controller
(CDC), it is very important for hosts to connect to CDCs with a consistent
source address. That's because of the way the CDC keeps track of all the
hosts that connect to it. In addition to the host NQN, the CDC also checks
the host IP address (the connection's source address) to uniquely identify
a host. This unique identifier is then used for fabric zoning.
With fabric zoning, administrators configure the list of I/O controllers
that a host is allowed to connect to. The CDC sends the list of I/O
controllers to the host in response to a Get Discovery Log Page (DLP)
command from the host. If a host does not connect to the CDC with the
right source address, it will receive invalid DLP entries (wrong zone).
This will cause the host to connect to the wrong I/O controllers.
libnvme tries to avoid making duplicate connections to the same
controller. This avoids consuming precious kernel resources. When an
application requests libnvme to connect to a controller (the candidate
controller), libnvme scans the sysfs to see if an existing connection
matches the candidate. If a matching connection is found, libnvme just
reuses it instead of creating a new one.
Matching the 3 mandatory parameters (transport, traddr, trsvcid) between
existing connections and a candidate connection is easy because they can
never be NULL and can therefore be compared. It is not the case for the
host-traddr and host-iface. These optional parameters can be NULL. A NULL
host-traddr or host-iface means that we have left it to the kernel to
determine the interface and source address to use for the connection.
Therefore, if we want to compare a non-NULL candidate host-traddr to
an existing connection with a NULL host-traddr, we cannot just compare the
two. They will obviously be different. Instead, we need to check the
src_addr of the existing connection to see if it matches the candidate's
host-traddr.
Prior to this patch, libnvme performed a simple string comparison
between the candidate's host-traddr (or host-iface) and the existing
connection's host-traddr (or host-iface). A match would be declared if
both were the same (including both NULL). Also, a match would even be
declared if the existing connection's host-traddr (or host-iface) was
NULL while the candidate's host-traddr (or host-iface) was non-NULL.
This is wrong and can lead to the wrong connections being reused and
the wrong DLP entries returned by the CDC.
With this patch, when a candidate wants a specific source address
(host-traddr != NULL) or interface (host-iface != NULL), libnvme will
now check the src_addr of each existing connection to ensure a 100%
match. If the src_addr is not available (kernel older than 6.1), then we
can still infer the real interface and source address of an existing
connection, if the existing connection has either its host-traddr or
host-iface defined (check code to see how it's done).
It's only when an exsiting connection's src_addr, host-iface, and
host-traddr are all NULL that we cannot clearly match to a candidate.
When that happens, libnvme will take an optimistic approach and
declare a match even though it doesn't have enough info to do so.
This "optimistic match" follows what libnvme was doing prior to this
patch.
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Daniel Wagner [Sat, 19 Aug 2023 10:44:07 +0000 (12:44 +0200)]
mi: remove nsid from nvme_mi_admin_identify_secondary_ctrl_list()
According to the NVMe specification, Identify CNS value 15h
("Secondary Controller list of controllers associated with the primary
controller processing the command") does not use the NSID field. So
remove the "nsid" argument from
nvme_mi_admin_identify_secondary_ctrl_list().
Fixes: 07b63103878 ("ioctl: remove nsid from nvme_identify_secondary_ctrl_list()") Signed-off-by: Daniel Wagner <dwagner@suse.de>
Caleb Sander [Sat, 12 Aug 2023 20:50:47 +0000 (14:50 -0600)]
test: add tests for Identify functions
Use the mock ioctl() infrastructure to test the functions in ioctl.h
that issue Identify commands.
nvme_identify_ns_csi_user_data_format() and
nvme_identify_iocs_ns_csi_user_data_format() are omitted
since they're not defined in the NVMe specification yet
Functions tested indirectly from other functions are also omitted.
projects / users / sagi / libnvme.git / log