linux-kernel - Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

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

Message-ID:
 <BY5PR04MB6849799468BC4F5118343015BC5CA@BY5PR04MB6849.namprd04.prod.outlook.com>
Date: Tue, 22 Jul 2025 17:34:56 +0000
From: Kamaljit Singh <Kamaljit.Singh1@....com>
To: Hannes Reinecke <hare@...e.de>, "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: "cassel@...nel.org" <cassel@...nel.org>, "dlemoal@...nel.org"
	<dlemoal@...nel.org>
Subject: Re: [PATCH v3 1/2] nvme: add capability to connect to an
 administrative controller

Hi Hannes,
 
From: Hannes Reinecke <hare@...e.de>
Date: Tuesday, July 22, 2025 at 00:06
>> Suggested-by: Niklas Cassel <cassel@...nel.org>
>>
>> Add capability to connect to an administrative controller by
>> preventing ioq creation for admin-controllers.
>>
>> * Add helper nvme_admin_ctrl() to check for an administrative controller
>> * Add helper nvme_override_prohibited_io_queues() to override queue_count
>> * Call nvme_override_prohibited_io_queues() from nvme_init_ctrl_finish()
>>    so it applies to nvme/tcp and nvme/rdma
>>
>> Signed-off-by: Kamaljit Singh <kamaljit.singh1@....com>
>> ---
>>   drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index e533d791955d..105127638c31 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3149,6 +3149,21 @@ static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
>>       return ctrl->opts && ctrl->opts->discovery_nqn;
>>   }
>>
>> +static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
>> +{
>> +     return ctrl->cntrltype == NVME_CTRL_ADMIN;
>> +}
>> +
>> +/*
>> + * An admin controller has one admin queue, but no I/O queues.
>> + * Override queue_count so it only creates an admin queue.
>> + */
>> +static inline void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>> +{
>> +     if (nvme_admin_ctrl(ctrl))
>> +             ctrl->queue_count = 1;
>> +}
>> +
>>   static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>>               struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>   {
>> @@ -3670,6 +3685,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>>       if (ret)
>>               return ret;
>>
>> +     if (nvme_admin_ctrl(ctrl))
>> +             dev_dbg(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     ctrl->subsys->subnqn);
>> +     nvme_override_prohibited_io_queues(ctrl);
>> +
>>       ret = nvme_configure_apst(ctrl);
>>       if (ret < 0)
>>               return ret;
>
>One wonders why this is done for admin controllers only; surely
>discovery controllers also don't support I/O queues, and should
>therefore have the same setting?
I agree. That was my patch-3 in v2, which I retracted in v3 based on
your comment below. Is someone taking that action item to discuss at FMDS?
I haven't attended it in a while.

If everyone agrees, I can add patch-3 back in and create v5 or since v4
patch-1 was accepted, I can issue a standalone patch just for this change.
Please let me know.



>From: Hannes Reinecke <hare@...e.de>
>Date: Thursday, July 3, 2025 at 02:42
>> Prevent ioq creation for discovery-controllers as the spec prohibits
>> them, similarly to the administrative controllers.
>>
>> Reference: NVMe Base rev 2.2, sec 3.1.3.4, fig 28.
>>
>While this might be true, we already deal with discovery controllers
>just fine, and never had issues with I/O queues being created.
>Presumably because discovery controllers never exposed I/O queues
>in the first place.
>
>Which also means that admin controllers should work already if they
>would not expose I/O queues.
BTW, the reason admin controllers didn't work without my patch is that
nvme-cli "nvme connect" doesn't allow "--nr-io-queues=0" and the default
=1 forces the kernel to create an I/O queue, which my patch overrides.

>
>We _might_ reduce the number of queues to '1' in nvme_set_queue_count(),
>but then we should issue a warning here as this would be a configuration
>error on the target side.
>
>'Prohibited' cuts both ways; the host shouldn't ask for it and the
>controller shouldn't advertise it...
>
>Or that's at least my reading. Might be worthwhile clarifying this
>at FMDS.

Regards,
Kamaljit