linux-kernel - Re: [PATCH v1 1/1] nvme: add admin controller support. prohibit ioq creation for admin & disco ctrlrs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <MN2PR04MB68626A7C2D254018FB37851DBCAC2@MN2PR04MB6862.namprd04.prod.outlook.com>
Date: Tue, 1 Apr 2025 22:57:36 +0000
From: Kamaljit Singh <Kamaljit.Singh1@....com>
To: Keith Busch <kbusch@...nel.org>, Niklas Cassel <cassel@...nel.org>
CC: "axboe@...nel.dk" <axboe@...nel.dk>, hch <hch@....de>, "sagi@...mberg.me"
	<sagi@...mberg.me>, "linux-nvme@...ts.infradead.org"
	<linux-nvme@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Damien Le Moal <dlemoal@...nel.org>, Niklas
 Cassel <cassel@...nel.org>
Subject: Re: [PATCH v1 1/1] nvme: add admin controller support. prohibit ioq
 creation for admin & disco ctrlrs

Hi Niklas & Keith,
 
On 2025/04/01 01:04, Niklas Cassel wrote:
 
>> > +   /* An admin or discovery controller has one admin queue, but no I/O queues */
>> > +   if (nvme_admin_ctrl(&ctrl->ctrl) || nvmf_discovery_ctrl(&ctrl->ctrl)) {
>> > +           ctrl->ctrl.queue_count = 1;
>> > +   } else if (ctrl->ctrl.queue_count < 2) {
>> > +           /* I/O controller with no I/O queues is not allowed */
>> > +           ret = -EOPNOTSUPP;
>> > +           dev_err(ctrl->ctrl.device,
>> > +                   "I/O controller doesn't allow zero I/O queues!\n");
>> > +           goto destroy_admin;
>> > +   }
>> 
>> The queue_count comes from the user. If the user provides a bad value
>> for an IO controller, you're erroring. If they provide a bad value for a
>> discovery or admin controller, you override the value. Why the different
>> behavior?
>> 
> 
> Good question.
Keith,
Yeah, that was me trying to plug the driver hole when I was hacking the
nvme-cli to pass a zero I/O queue value for an admin-controller. While
doing that allowed us to connect to an admin-controller, it didn't prevent
us from connecting to an I/O controller with zero I/O queues. Per the
spec that shouldn't be allowed. Hope that clarifies the reason for that
2nd change. I'll make it into its own patch.
 
 
> 
> My initial proposal was simply to override the user provided value
> to 1 (admin queue only) in case of admin (or discovery) controller.
>
> The check for queue_count < 2 should be in a separate patch, if we
> want that check at all.
Yes, I'll make that into a separate patch.
 
> But to be honest, the code did previously
> allow an I/O controller with just the admin queue and no I/O queues.
Agree. Initially, I was able to use that hole by forcing nvme-cli to
allow zero IOQs. But based on your suggested driver change we don't
need to patch nvme-cli anymore. That's slick!
 
However, from the nvme-cli's perspective it does feel awkward to be forced
by "nvme connect" to use -i <non-zero> for an admin-controller, even though
its now being overridden with this patch. We will have to come up with a
cleaner and standardized way to connect to an admin controller.  A standard
port number for an admin controller might be the way to go but it's not in
the spec yet.
 
 
> Thus, without a commit message explaining clearly why we should start
> to disallow an I/O controller with just the admin queue, I think that
> additional check is wrong.
For the separate patch you suggested, I'll add comments to make it clearer.
 
Thanks,
Kamaljit