Re: git: 66b5296f1b29 - main - ctld: Add support for NVMe over Fabrics
Date: Tue, 14 Apr 2026 12:35:52 UTC
On Tue, Apr 14, 2026 at 4:31 AM John Baldwin <jhb@freebsd.org> wrote:
>
> On 4/13/26 13:36, Alan Somers wrote:
> > On Mon, Apr 13, 2026 at 10:56 AM John Baldwin <jhb@freebsd.org> wrote:
> >>
> >> 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.
> >
> > Excuse me, I spoke a little too soon. You are correct that ctld has
> > been converting target names to lower case before registering them in
> > the kernel for a long time. The change is that previously, if an
> > initiator attempted to connect to an uppercase target name, ctld would
> > accept it. That's because port_find_in_pg used strcasecmp in
> > stable/14. But change 4b1aac931465f39c5c26bfa1d5539a428d340f20
> > removed strcasecmp, replacing it by the C++ STL's find method on
> > std::unordered_map.
> >
> > So we used to accept connections case-insensitively, and now we accept
> > them case-sensitively. To restore the previous behavior, should we
> > add tolower() on the target_name in iscsi_connection::login() ?
>
> Yes, we should normalize there, and that indeed is my fault and warrants
> a Fixes tag.
>
> --
> John Baldwin
Ok. I'll take care of it.