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>] [day] [month] [year] [list]

Message-ID:
 <MN2PR04MB68620030DBBDB25900DE3E82BCAC2@MN2PR04MB6862.namprd04.prod.outlook.com>
Date: Tue, 1 Apr 2025 22:47:29 +0000
From: Kamaljit Singh <Kamaljit.Singh1@....com>
To: Damien Le Moal <dlemoal@...nel.org>, "kbusch@...nel.org"
	<kbusch@...nel.org>, "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>
CC: Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v1 1/1] nvme: add admin controller support. prohibit ioq
 creation for admin & disco ctrlrs

Hi Damien,
 
On 2025/03/28 15:09, Damien Le Moal wrote:
>> Added capability to connect to an administrative controller by
> 
>s/Added/Add
Done
 
>> preventing ioq creation for admin-controllers. Also prevent ioq creation
>> for discovery-controllers as the spec prohibits them.
 
>This second part should be a different (preparatory) patch.
I've separated it as its own patch. Sorry took a while to procure the setup
But I’ve tested it now. Will share updated patch-set.
 
You were right, it is a pain using Outlook for kernel work :(
Vim to the rescue for now.
 
>> 
>> * Added nvme_admin_ctrl() to check for an administrative controller
 
>s/Added/Add
>And this should be a different preparatory patch.
Sounds good.
 
>> 
>> * Renamed nvme_discovery_ctrl() to nvmf_discovery_ctrl() as discovery is
>>   more relevant to fabrics
 
>I do not think that is necessary since this is testing the controller type,
>which may be limited to fabrics or not.
I undid that rename and moved the function as Niklas suggested, to core.c.
I tried moving nvme_discovery_ctrl(), nvme_admin_ctrl &
nvme_update_ctrl_queue_count() to host/nvme.h but couldn't since target/
passthru.c includes that host header & fails compilation.
 
To make it work that way we would have to include host/fabrics.h in target/
passthru.c but that's causing a lot of unresolved symbols and requires a
different/larger commit. Also, it may not even be desirable as it will further
pollute the target code with host specific header.  Please comment.
 
 
>> @@ -2863,13 +2858,19 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>       else
>>               subsys->subtype = NVME_NQN_NVME;
>> 
>> -     if (nvme_discovery_ctrl(ctrl) && subsys->subtype != NVME_NQN_DISC) {
>> +     if (nvmf_discovery_ctrl(ctrl) && subsys->subtype != NVME_NQN_DISC) {
>>               dev_err(ctrl->device,
>>                       "Subsystem %s is not a discovery controller",
>>                       subsys->subnqn);
>>               kfree(subsys);
>>               return -EINVAL;
>>       }
>> +     if (nvme_admin_ctrl(ctrl)) {
>> +             dev_info(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     subsys->subnqn);
>> +     }
 
>Is this really necessary ? In any case, please remove the curly brackets.
I've removed it. I had started off by adding a dev_err check similar to
the one for discovery ctrl above it but since there is no unique identifier
(like the SUBNQN or unique port) for an admin controller, I turned it into
a dev_info to at least announce the presence of an admin controller. Any
suggestions for error validation at init time?
 
 
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 299e3c19df9d..0f3150411bd5 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1030,6 +1030,17 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>>               goto destroy_admin;
>>       }
>> 
>> +     /* 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;
>> +     }
 
>This is identical to the change for tcp, so maybe make this a helper function ?
I've converted it into nvme_update_ctrl_queue_count() and added it to host/nvme.h.
Will be in the next patch-set.