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

Message-ID: <ca53c3a8-8af2-4e34-b0e6-3571cbec4bee@kernel.org>
Date: Fri, 28 Mar 2025 15:09:13 -0700
From: Damien Le Moal <dlemoal@...nel.org>
To: Kamaljit Singh <kamaljit.singh1@....com>,
 "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 <Niklas.Cassel@....com>
Subject: Re: [PATCH v1 1/1] nvme: add admin controller support. prohibit ioq
 creation for admin & disco ctrlrs

On 2025/03/28 14:36, Kamaljit Singh wrote:
> Added capability to connect to an administrative controller by

s/Added/Add

> 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.

> 
> * Added nvme_admin_ctrl() to check for an administrative controller

s/Added/Add
And this should be a different preparatory patch.

> 
> * 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.

> * Similar to a discovery ctrl, prevent an admin-ctrl from getting a
>   smart log (LID=2). LID 2 is optional for admin controllers but in the
>   future when we add support for the newly added LID=0 (supported log
>   pages), we can make GLP accesses smarter by basing such calls on
>   response from LID=0 reads.
> 
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@....com>

[...]

> @@ -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.

[...]

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

> +
>  	if (ctrl->ctrl.opts->queue_size > ctrl->ctrl.sqsize + 1) {
>  		dev_warn(ctrl->ctrl.device,
>  			"queue_size %zu > ctrl sqsize %u, clamping down\n",
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 644f84284b6f..3fe2f617bfd5 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2199,6 +2199,17 @@ static int nvme_tcp_setup_ctrl(struct nvme_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) || nvmf_discovery_ctrl(ctrl)) {
> +		ctrl->queue_count = 1;
> +	} else if (ctrl->queue_count < 2) {
> +		/* I/O controller with no I/O queues is not allowed */
> +		ret = -EOPNOTSUPP;
> +		dev_err(ctrl->device,
> +			"I/O controller doesn't allow zero I/O queues!\n");
> +		goto destroy_admin;
> +	}
> +
>  	if (opts->queue_size > ctrl->sqsize + 1)
>  		dev_warn(ctrl->device,
>  			"queue_size %zu > ctrl sqsize %u, clamping down\n",

-- 
Damien Le Moal
Western Digital Research