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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250403044901.GD22803@lst.de>
Date: Thu, 3 Apr 2025 06:49:01 +0200
From: Christoph Hellwig <hch@....de>
To: Keith Busch <kbusch@...nel.org>
Cc: Kamaljit Singh <kamaljit.singh1@....com>, axboe@...nel.dk, hch@....de,
	sagi@...mberg.me, linux-nvme@...ts.infradead.org,
	linux-kernel@...r.kernel.org, niklas.cassel@....com,
	damien.lemoal@....com
Subject: Re: [PATCH v1 1/1] nvme: add admin controller support. prohibit
 ioq creation for admin & disco ctrlrs

On Mon, Mar 31, 2025 at 09:03:11AM -0600, Keith Busch wrote:
> On Fri, Mar 28, 2025 at 02:36:40PM -0700, Kamaljit Singh wrote:
> > -static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
> > -{
> > -	return ctrl->opts && ctrl->opts->discovery_nqn;
> > -}
> > -
> 
> I suppose it's fine to rename this function with an nvmf_ prefix, but
> it's not really related to the rest of the patch and makes the diff
> larger than necessary.

It isn't, nvmf_ is really for code in the fabrics library and not used
by core code.

> > +	/* 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?

Also we historically do allow I/O controllers without I/O queues to
allow for various kinds of recovery action.  So I don't think we
should change that.