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

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() ?