Re: git: 66b5296f1b29 - main - ctld: Add support for NVMe over Fabrics

On 4/13/26 11:51, Alan Somers wrote:
> On Wed, Aug 6, 2025 at 2:10 PM John Baldwin <jhb@freebsd.org> wrote:
>>
>> The branch main has been updated by jhb:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/?id=66b5296f1b29083634e2875ff08c32e7b6b866a8
>>
>> commit 66b5296f1b29083634e2875ff08c32e7b6b866a8
>> Author:     John Baldwin <jhb@FreeBSD.org>
>> AuthorDate: 2025-08-06 19:57:50 +0000
>> Commit:     John Baldwin <jhb@FreeBSD.org>
>> CommitDate: 2025-08-06 19:59:13 +0000
>>
>>      ctld: Add support for NVMe over Fabrics
>>
>>      While the overall structure is similar for NVMeoF controllers and
>>      iSCSI targets, there are sufficient differences that NVMe support uses
>>      an alternate configuration syntax.
>>
>>      - In authentication groups, permitted NVMeoF hosts can be allowed by
>>        names (NQNs) via "host-nqn" values (similar to "initiator-name" for
>>        iSCSI).  Similarly, "host-address" accepts permitted host addresses
>>        similar to "initiator-portal" for iSCSI.
>>
>>      - A new "transport-group" context enumerates transports that can be
>>        used by a group of NVMeoF controllers similar to the "portal-group"
>>        context for iSCSI.  In this section, the "listen" keyword accepts a
>>        transport as well as an address to permit other types of transports
>>        besides TCP in the future.  The "foreign", "offload", and "redirect"
>>        keywords are also not meaningful and thus not supported.
>>
>>      - A new "controller" context describes an NVMeoF I/O controller
>>        similar to the "target" context for iSCSI.  One key difference here
>>        is that "lun" objects are replaced by "namespace" objects.  However,
>>        a "namespace" can reference a named global lun permitting LUNs to be
>>        shared between iSCSI targets and NVMeoF controllers.
>>
>>      NB: Authentication via CHAP is not implemented for NVMeoF.
>>
>>      Reviewed by:    imp
>>      Sponsored by:   Chelsio Communications
>>      Differential Revision:  https://reviews.freebsd.org/D48773
> ...
>> +struct target *
>> +conf::add_controller(const char *name)
>> +{
>> +       if (!nvmf_nqn_valid_strict(name)) {
>> +               log_warnx("controller name \"%s\" is invalid for NVMe", name);
>> +               return nullptr;
>> +       }
>> +
>> +       /*
>> +        * Normalize the name to lowercase to match iSCSI.
>> +        */
>> +       std::string t_name(name);
>> +       for (char &c : t_name)
>> +               c = tolower(c);
> ...
> 
> This makes it impossible to define an uppercase or mixed case target
> name in ctld.  I guess the intent was to comply with rfc3722[^1]?
> Even so, it's surprising, because such target names used to work.
> It's also inconsistent, because it's still possible to create an
> uppercase target name using ctladm directly, like this:
> 
> ctladm port -c -d iscsi -O cfiscsi_portal_group_tag=257 -O
> cfiscsi_target=iqn.2018-10.myhost:TESTVOL1
> 
> Should we warn the user if they specify an uppercase target name, or
> even fail to create it?
> 
> [^1]: https://datatracker.ietf.org/doc/html/rfc3722

Note that this function is for NVMe, not iSCSI.  iSCSI targets are created in
conf::add_target which has similar code:

struct target *
conf::add_target(const char *name)
{
	if (!valid_iscsi_name(name, log_warnx))
		return (nullptr);

	/*
	 * RFC 3722 requires us to normalize the name to lowercase.
	 */
	std::string t_name(name);
	for (char &c : t_name)
		c = tolower(c);

Prior to the C++ commit, this change was already in place:

struct target *
target_new(struct conf *conf, const char *name)
{
	struct target *targ;
	int i, len;

	targ = target_find(conf, name);
	if (targ != NULL) {
		log_warnx("duplicated target \"%s\"", name);
		return (NULL);
	}
	if (valid_iscsi_name(name, log_warnx) == false) {
		return (NULL);
	}
	targ = new target();
	targ->t_name = checked_strdup(name);

	/*
	 * RFC 3722 requires us to normalize the name to lowercase.
	 */
	len = strlen(name);
	for (i = 0; i < len; i++)
		targ->t_name[i] = tolower(targ->t_name[i]);

	targ->t_conf = conf;
	TAILQ_INSERT_TAIL(&conf->conf_targets, targ, t_next);

	return (targ);
}

This was present in commit 009ea47eb2d21856af4529aaaca32cd67748daea
which brought in the iSCSI target, so it has always been present
in ctld.

Also, AFAICT, the names are still accepted, they are just normalized.

I guess one difference is that before, target_new() called target_find()
with the non-normalized name to check for duplicates, and now we check
for duplicates after normalizing the name.  I'm not sure how that worked
in the past in practice as you would have had two targets with the same
name (e.g. I wonder what the ctladm portlist output looked like for this
case and if it would have listed two ports with the same name)?  I suspect
that was more by accident and probably didn't work properly in practice
(e.g. the kernel handoff ioctl used the normalized name when invoking
CTL_ISCSI, so connections to both "names" probably were always mapped to
only one of the connections, and finding a port during login processing
probably only found the first target, and only if the initiator gave the
all-lowercase name).

That is to say, you didn't get an error before, but it didn't work, and
now it tells you that it doesn't work AFAICT.

-- 
John Baldwin