Date: Tue, 14 Apr 2026 09:59:07 -0600 From: Alan Somers <asomers@freebsd.org> Cc: John Baldwin <jhb@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 66b5296f1b29 - main - ctld: Add support for NVMe over Fabrics Message-ID: <CAOtMX2hmv%2B_nAU0DAaxn_TqaTMZL5hc-X9GS2swp%2B5FbDOHPzQ@mail.gmail.com> In-Reply-To: <CAOtMX2iGvoeBGqLfaTYsJ=H283d%2B8aeor_A-tg-OTLgi3SPZqA@mail.gmail.com> References: <202508062010.576KA2Mk062184@gitrepo.freebsd.org> <CAOtMX2hxS%2BhsaQya0ndNEGyVCYss-PP_wUmz_baErbCECKBJJw@mail.gmail.com> <a441501d-89f8-4a5d-b32b-f63db1229724@FreeBSD.org> <CAOtMX2jTpD7VWEHP%2BaAK_WD%2BKBMh3B18B-2S9AdokenSjHvEqg@mail.gmail.com> <680f1cb7-f8f6-4d40-af31-83e2ea0205d6@FreeBSD.org> <CAOtMX2iGvoeBGqLfaTYsJ=H283d%2B8aeor_A-tg-OTLgi3SPZqA@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
On Tue, Apr 14, 2026 at 6:35 AM Alan Somers <asomers@freebsd.org> wrote: > > 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. FYI, I opened this bug, just so other users can find the issue. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=294522home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2hmv%2B_nAU0DAaxn_TqaTMZL5hc-X9GS2swp%2B5FbDOHPzQ>