Re: [PATCH 69/98] nvmet: confine fatal errors of unauthenticated queues

[Hannes Reinecke <hare@suse.de>]

On 10/28/26 5:42 PM, Jens Axboe wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> The Connect of an I/O queue names its controller by NQNs and cntlid and
> needs no secret, DH-HMAC-CHAP comes after it. But a fatal error on any
> queue, a failed authentication or a transport error like an unknown PDU
> type, takes the whole controller down. So anyone who can reach the port
> and knows the NQNs can attach a queue to an authenticated host's
> controller and kick that host out, repeatedly.
> 
> Add a ->disconnect_queue() transport op and, for an I/O queue that
> hasn't passed the authentication asked of it, disconnect just that
> queue. Transports without the op (fc, loop) take the controller down as
> before. A queue that had authenticated and fails to do so again is the
> host's own, there the controller goes down as before too.
> 
> [axboe: resolved against the per-queue auth_lock and expiry work that
>   is upstream now]
> 
> Fixes: db1312dd9548 ("nvmet: implement basic In-Band Authentication")
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Chaitanya Kulkarni <kch@nvidia.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   drivers/nvme/target/auth.c             |  9 ++++++---
>   drivers/nvme/target/core.c             | 16 ++++++++++++++++
>   drivers/nvme/target/fabrics-cmd-auth.c | 20 ++++++++++++++++----
>   drivers/nvme/target/nvmet.h            |  7 +++++++
>   drivers/nvme/target/rdma.c             |  9 ++++++++-
>   drivers/nvme/target/tcp.c              | 11 ++++++++++-
>   6 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index 5809ce32a2e4..335cf8f3f79e 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -261,10 +261,8 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl)
>   #endif
>   }
>   
> -bool nvmet_check_auth_status(struct nvmet_req *req)
> +bool nvmet_sq_auth_status(struct nvmet_sq *sq)
>   {
> -	struct nvmet_sq *sq = req->sq;
> -
>   	if (!sq->ctrl->host_key || sq->authenticated)
>   		return true;
>   
> @@ -272,6 +270,11 @@ bool nvmet_check_auth_status(struct nvmet_req *req)
>   	return sq->qid && nvmet_queue_tls_keyid(sq);
>   }
>   
> +bool nvmet_check_auth_status(struct nvmet_req *req)
> +{
> +	return nvmet_sq_auth_status(req->sq);
> +}
> +
>   int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
>   			 unsigned int shash_len)
>   {
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index d01f8abed66c..eea67fba6f92 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1828,6 +1828,22 @@ void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvmet_ctrl_fatal_error);
>   
> +/*
> + * A fatal error on one queue.  The Connect command of an I/O queue needs no
> + * secret, so until the queue has authenticated it may be anybody's: do not
> + * let it take down the controller of the host whose NQN it has named.
> + */
> +void nvmet_sq_fatal_error(struct nvmet_sq *sq)
> +{
> +	struct nvmet_ctrl *ctrl = sq->ctrl;
> +
> +	if (sq->qid && ctrl->ops->disconnect_queue && !nvmet_sq_auth_status(sq))
> +		ctrl->ops->disconnect_queue(sq);
> +	else
> +		nvmet_ctrl_fatal_error(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(nvmet_sq_fatal_error);
> +
>   ssize_t nvmet_ctrl_host_traddr(struct nvmet_ctrl *ctrl,
>   		char *traddr, size_t traddr_len)
>   {
> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> index 8c48140eb542..cb6be285b3c4 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -33,6 +33,14 @@ static void nvmet_auth_expired_work(struct work_struct *work)
>   	mutex_unlock(&sq->auth_lock);
>   }
>   
> +static void nvmet_auth_fatal_error(struct nvmet_sq *sq, bool passed)
> +{
> +	if (passed)
> +		nvmet_ctrl_fatal_error(sq->ctrl);
> +	else
> +		nvmet_sq_fatal_error(sq);
> +}
> +
>   void nvmet_auth_sq_init(struct nvmet_sq *sq)
>   {
>   	/* Initialize in-band authentication */
> @@ -253,11 +261,13 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
>   	void *d;
>   	u32 tl;
>   	u16 status = 0;
> +	bool passed;
>   	u8 dhchap_status;
>   
>   	mutex_lock(&req->sq->auth_lock);
>   	mutex_lock(&ctrl->auth_lock);
> -
> +	/* A queue that fails to authenticate again is no stranger's. */
> +	passed = nvmet_sq_auth_status(req->sq);
>   	if (req->cmd->auth_send.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
>   		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>   		req->error_loc =
> @@ -430,7 +440,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
>   	/* Final states, clear up variables */
>   	nvmet_auth_sq_free(req->sq);
>   	if (req->sq->dhchap_step == NVME_AUTH_DHCHAP_MESSAGE_FAILURE2)
> -		nvmet_ctrl_fatal_error(ctrl);
> +		nvmet_auth_fatal_error(req->sq, passed);
>   
>   complete:
>   	mutex_unlock(&ctrl->auth_lock);
> @@ -551,10 +561,12 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
>   	void *d;
>   	u32 al;
>   	u16 status = 0;
> +	bool passed;
>   
>   	mutex_lock(&req->sq->auth_lock);
>   	mutex_lock(&ctrl->auth_lock);
> -
> +	/* A queue that fails to authenticate again is no stranger's. */
> +	passed = nvmet_sq_auth_status(req->sq);
>   	if (req->cmd->auth_receive.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
>   		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>   		req->error_loc =
> @@ -641,7 +653,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
>   		nvmet_auth_sq_free(req->sq);
>   	else if (req->sq->dhchap_step == NVME_AUTH_DHCHAP_MESSAGE_FAILURE1) {
>   		nvmet_auth_sq_free(req->sq);
> -		nvmet_ctrl_fatal_error(ctrl);
> +		nvmet_auth_fatal_error(req->sq, passed);
>   	}
>   	mutex_unlock(&ctrl->auth_lock);
>   	mutex_unlock(&req->sq->auth_lock);
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index cc7fface77de..e354a71514ae 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -441,6 +441,7 @@ struct nvmet_fabrics_ops {
>   	int (*add_port)(struct nvmet_port *port);
>   	void (*remove_port)(struct nvmet_port *port);
>   	void (*delete_ctrl)(struct nvmet_ctrl *ctrl);
> +	void (*disconnect_queue)(struct nvmet_sq *sq);
>   	void (*disc_traddr)(struct nvmet_req *req,
>   			struct nvmet_port *port, char *traddr);
>   	ssize_t (*host_traddr)(struct nvmet_ctrl *ctrl,
> @@ -622,6 +623,7 @@ void nvmet_sq_destroy(struct nvmet_sq *sq);
>   int nvmet_sq_init(struct nvmet_sq *sq, struct nvmet_cq *cq);
>   
>   void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl);
> +void nvmet_sq_fatal_error(struct nvmet_sq *sq);
>   
>   void nvmet_update_cc(struct nvmet_ctrl *ctrl, u32 new);
>   
> @@ -989,6 +991,7 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl);
>   void nvmet_auth_sq_free(struct nvmet_sq *sq);
>   void nvmet_auth_sq_destroy(struct nvmet_sq *sq);
>   int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id);
> +bool nvmet_sq_auth_status(struct nvmet_sq *sq);
>   bool nvmet_check_auth_status(struct nvmet_req *req);
>   int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
>   			 unsigned int hash_len);
> @@ -1015,6 +1018,10 @@ static inline void nvmet_auth_sq_init(struct nvmet_sq *sq)
>   static inline void nvmet_destroy_auth(struct nvmet_ctrl *ctrl) {};
>   static inline void nvmet_auth_sq_free(struct nvmet_sq *sq) {};
>   static inline void nvmet_auth_sq_destroy(struct nvmet_sq *sq) {};
> +static inline bool nvmet_sq_auth_status(struct nvmet_sq *sq)
> +{
> +	return true;
> +}
>   static inline bool nvmet_check_auth_status(struct nvmet_req *req)
>   {
>   	return true;
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index ffa1e172c259..c4b848f84465 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -687,7 +687,7 @@ static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp)
>   static void nvmet_rdma_error_comp(struct nvmet_rdma_queue *queue)
>   {
>   	if (queue->nvme_sq.ctrl) {
> -		nvmet_ctrl_fatal_error(queue->nvme_sq.ctrl);
> +		nvmet_sq_fatal_error(&queue->nvme_sq);
>   	} else {
>   		/*
>   		 * we didn't setup the controller yet in case
> @@ -1848,6 +1848,12 @@ static int nvmet_rdma_cm_handler(struct rdma_cm_id *cm_id,
>   	return ret;
>   }
>   
> +static void nvmet_rdma_disconnect_queue(struct nvmet_sq *sq)
> +{
> +	nvmet_rdma_queue_disconnect(container_of(sq, struct nvmet_rdma_queue,
> +						 nvme_sq));
> +}
> +
>   static void nvmet_rdma_delete_ctrl(struct nvmet_ctrl *ctrl)
>   {
>   	struct nvmet_rdma_queue *queue, *n;
> @@ -2078,6 +2084,7 @@ static const struct nvmet_fabrics_ops nvmet_rdma_ops = {
>   	.remove_port		= nvmet_rdma_remove_port,
>   	.queue_response		= nvmet_rdma_queue_response,
>   	.delete_ctrl		= nvmet_rdma_delete_ctrl,
> +	.disconnect_queue	= nvmet_rdma_disconnect_queue,
>   	.disc_traddr		= nvmet_rdma_disc_port_addr,
>   	.host_traddr		= nvmet_rdma_host_port_addr,
>   	.get_mdts		= nvmet_rdma_get_mdts,
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index a2a5f39d2bb7..c8f0c52a1b95 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -412,7 +412,7 @@ static void nvmet_tcp_socket_error(struct nvmet_tcp_queue *queue, int status)
>   	if (status == -EPIPE || status == -ECONNRESET || !queue->nvme_sq.ctrl)
>   		kernel_sock_shutdown(queue->sock, SHUT_RDWR);
>   	else
> -		nvmet_ctrl_fatal_error(queue->nvme_sq.ctrl);
> +		nvmet_sq_fatal_error(&queue->nvme_sq);
>   }
>   
>   static int nvmet_tcp_map_data(struct nvmet_tcp_cmd *cmd)
> @@ -2223,6 +2223,14 @@ static void nvmet_tcp_remove_port(struct nvmet_port *nport)
>   	kfree(port);
>   }
>   
> +static void nvmet_tcp_disconnect_queue(struct nvmet_sq *sq)
> +{
> +	struct nvmet_tcp_queue *queue =
> +		container_of(sq, struct nvmet_tcp_queue, nvme_sq);
> +
> +	kernel_sock_shutdown(queue->sock, SHUT_RDWR);
> +}
> +
>   static void nvmet_tcp_delete_ctrl(struct nvmet_ctrl *ctrl)
>   {
>   	struct nvmet_tcp_queue *queue;
> @@ -2300,6 +2308,7 @@ static const struct nvmet_fabrics_ops nvmet_tcp_ops = {
>   	.remove_port		= nvmet_tcp_remove_port,
>   	.queue_response		= nvmet_tcp_queue_response,
>   	.delete_ctrl		= nvmet_tcp_delete_ctrl,
> +	.disconnect_queue	= nvmet_tcp_disconnect_queue,
>   	.install_queue		= nvmet_tcp_install_queue,
>   	.disc_traddr		= nvmet_tcp_disc_port_addr,
>   	.host_traddr		= nvmet_tcp_host_port_addr,

No reason why we shouldn't to this for FC.
I'll update the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich