Re: [PATCH v3 2/2] nvme: expose queue information via debugfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] nvme-tcp: race with queue teardown in nvme_tcp_print_io_queue_info()
- [Medium] nvme: debugfs: nvme_io_queue_info_start() skips queues on resumed reads
- [Low] nvme: debugfs: missing static qualifier on nvme_io_queue_info_seq_ops
--

commit 0f7d7347ae3aacfc41bb3e9a63226f587e2b9ffd
Author: Nilay Shroff <nilay@linux.ibm.com>

nvme: expose queue information via debugfs

This commit adds an io_queue_info debugfs attribute to expose per-queue
information for NVMe controllers using seq_file iteration. For NVMe-TCP,
it outputs each I/O queue's bound CPU and TCP socket source and
destination address and port.

> diff --git a/drivers/nvme/host/debugfs.c b/drivers/nvme/host/debugfs.c
> index 798fec4730f0a..8ea0413f7b753 100644
> --- a/drivers/nvme/host/debugfs.c
> +++ b/drivers/nvme/host/debugfs.c
> @@ -25,6 +25,61 @@ struct nvme_debugfs_ctx {
>  	struct nvme_debugfs_attr *attr;
>  };
>  
> +static void *nvme_io_queue_info_start(struct seq_file *m, loff_t *pos)
> +{
> +	struct nvme_debugfs_ctx *ctx = m->private;
> +	struct nvme_ns *ns = ctx->data;
> +	struct nvme_ctrl *ctrl = ns->ctrl;
> +
> +	/*
> +	 * IO queues starts at offset 1.
> +	 */
> +	return (++*pos < ctrl->queue_count) ? pos : NULL;
> +}

[Severity: Medium]
Will unconditionally pre-incrementing *pos in nvme_io_queue_info_start()
cause queues to be skipped when seq_read_iter() resumes reading on a
subsequent read() call or after a seq_file buffer overflow?

In fs/seq_file.c:seq_read_iter(), the Fill loop advances m->index via
m->op->next() before checking whether the user buffer is full or whether
m->op->show() overflowed the seq_file buffer:

	p = m->op->next(m, p, &m->index);
	...
	if (!p || IS_ERR(p))	// no next record for us
		break;
	if (m->count >= iov_iter_count(iter))
		break;
	err = m->op->show(m, p);
	if (err > 0) {		// ->show() says "skip it"
		m->count = offs;
	} else if (err || seq_has_overflowed(m)) {
		m->count = offs;
		break;
	}

When the loop stops because the seq_file buffer overflowed or the caller's
buffer was filled, m->index has already been incremented by
nvme_io_queue_info_next() to the next unshown queue ID. On the next read,
seq_read_iter() calls nvme_io_queue_info_start(m, &m->index), which
executes ++*pos a second time and skips that queue.

Should nvme_io_queue_info_start() only advance *pos when *pos is 0?

> +
> +static void *nvme_io_queue_info_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	struct nvme_debugfs_ctx *ctx = m->private;
> +	struct nvme_ns *ns = ctx->data;
> +	struct nvme_ctrl *ctrl = ns->ctrl;
> +
> +	return (++*pos < ctrl->queue_count) ? pos : NULL;
> +}
> +
> +static void nvme_io_queue_info_stop(struct seq_file *m, void *v)
> +{
> +	/* nothing to be done */
> +}
> +
> +static int nvme_io_queue_info_show(struct seq_file *m, void *v)
> +{
> +	struct nvme_debugfs_ctx *ctx = m->private;
> +	struct nvme_ns *ns = ctx->data;
> +	struct nvme_ctrl *ctrl = ns->ctrl;
> +
> +	if (ctrl->ops->print_io_queue_info)
> +		return ctrl->ops->print_io_queue_info(m, ctrl, *(loff_t *)v);
> +
> +	return 0;
> +}
> +
> +const struct seq_operations nvme_io_queue_info_seq_ops = {
> +	.start = nvme_io_queue_info_start,
> +	.next = nvme_io_queue_info_next,
> +	.stop = nvme_io_queue_info_stop,
> +	.show = nvme_io_queue_info_show
> +};

[Severity: Low]
Should nvme_io_queue_info_seq_ops be marked static since it is only used
within drivers/nvme/host/debugfs.c?

[ ... ]

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7190eb10c2481..a59169629d56b 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2691,6 +2691,59 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
>  	kfree(ctrl);
>  }
>  
> +static int nvme_tcp_print_io_queue_info(struct seq_file *m,
> +		struct nvme_ctrl *ctrl, int qid)
> +{
> +	int cpu;
> +	struct sockaddr_storage src, dst;
> +	struct nvme_tcp_ctrl *tctrl = to_tcp_ctrl(ctrl);
> +	struct nvme_tcp_queue *queue = &tctrl->queues[qid];
> +	int ret = -EINVAL;
> +
> +	if (!qid || qid >= ctrl->queue_count)
> +		return -EINVAL;
> +
> +	if (!test_bit(NVME_TCP_Q_LIVE, &queue->flags))
> +		return 0;
> +
> +	mutex_lock(&queue->queue_lock);
> +	if (!queue->sock)
> +		goto unlock;
> +
> +	ret = kernel_getsockname(queue->sock, (struct sockaddr *)&src);
> +	if (ret <= 0)
> +		goto unlock;
> +
> +	ret = kernel_getpeername(queue->sock, (struct sockaddr *)&dst);
> +	if (ret <= 0)
> +		goto unlock;

[Severity: High]
Can nvme_tcp_print_io_queue_info() race with queue teardown during
controller reset or error recovery?

Holding a reference on struct nvme_ns keeps the namespace and controller
allocated, but does not prevent nvme_reset_ctrl_work() or
nvme_tcp_error_recovery_work() from running nvme_tcp_teardown_io_queues()
while io_queue_info is being read.

In nvme_tcp_stop_queue_nowait(), NVME_TCP_Q_LIVE is cleared under
queue->queue_lock while queue->sock remains non-NULL:

	mutex_lock(&queue->queue_lock);
	if (test_and_clear_bit(NVME_TCP_Q_LIVE, &queue->flags))
		__nvme_tcp_stop_queue(queue);
	/* Stopping the queue will disable TLS */
	queue->tls_enabled = false;
	mutex_unlock(&queue->queue_lock);

Shortly after, nvme_tcp_free_io_queues() -> nvme_tcp_free_queue() runs
without holding queue->queue_lock, frees queue->sock before clearing the
pointer, and destroys queue->queue_lock:

	__fput_sync(queue->sock->file);
	queue->sock = NULL;
	memalloc_noio_restore(noio_flag);

	kfree(queue->pdu);
	mutex_destroy(&queue->send_mutex);
	mutex_destroy(&queue->queue_lock);
	mutex_destroy(&queue->pf_cache_lock);

If nvme_tcp_print_io_queue_info() checks test_bit(NVME_TCP_Q_LIVE,
&queue->flags) just before teardown, it can acquire queue->queue_lock
after nvme_tcp_stop_queue_nowait() unlocks it, see queue->sock != NULL
because NVME_TCP_Q_LIVE is not re-checked under the lock, and dereference
queue->sock in kernel_getsockname() and kernel_getpeername() concurrently
with __fput_sync(queue->sock->file), or touch queue->queue_lock while
nvme_tcp_free_queue() destroys it (and reconnect re-initializes it in
nvme_tcp_alloc_queue()).

In addition, if !queue->sock is taken under queue->queue_lock (where ret
is still -EINVAL) or if kernel_getpeername() returns -ENOTCONN on a
shut-down socket, returning a negative error code from the seq_file show
callback aborts the entire seq_read_iter() read instead of skipping the
stopped queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026123411.1988949-4-nilay@linux.ibm.com?part=2