Re: [PATCH v3 1/2] nvme: add debugfs helpers for NVMe drivers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvme: missing debugfs_mutex and NOIO scope in nvme_debugfs_create_files()
--

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

nvme: add debugfs helpers for NVMe drivers

This patch introduces helper APIs and an attribute structure for NVMe
drivers to register and unregister seq_file-based debugfs entries on a
request queue's debugfs directory.

> diff --git a/drivers/nvme/host/debugfs.c b/drivers/nvme/host/debugfs.c
> new file mode 100644
> index 0000000000000..798fec4730f0a
> --- /dev/null
> +++ b/drivers/nvme/host/debugfs.c
> @@ -0,0 +1,133 @@

[ ... ]

> +static void nvme_debugfs_create_files(struct request_queue *q,
> +		const struct nvme_debugfs_attr *attr, void *data)
> +{
> +	if (WARN_ON_ONCE(!q->debugfs_dir))
> +		return;
> +
> +	for (; attr->name; attr++) {
> +		if (attr->is_visible && !attr->is_visible(data))
> +			continue;
> +		debugfs_create_file_aux(attr->name, attr->mode, q->debugfs_dir,
> +				data, (void *)attr, &nvme_debugfs_fops);
> +	}
> +}

[Severity: Medium]
Should nvme_debugfs_create_files() hold q->debugfs_mutex and run under a
memalloc_noio_save() scope when accessing q->debugfs_dir and creating
debugfs files?

In include/linux/blkdev.h, struct request_queue documents that
debugfs_mutex serializes all debugfs metadata operations on debugfs_dir:

	struct dentry		*debugfs_dir;
	struct dentry		*sched_debugfs_dir;
	struct dentry		*rqos_debugfs_dir;
	/*
	 * Serializes all debugfs metadata operations using the above dentries.
	 */
	struct mutex		debugfs_mutex;

Furthermore, block/blk.h notes in blk_debugfs_lock_nomemsave() why NOIO
context is needed alongside debugfs_mutex (as introduced in commit
dfe48ea17973 ("blk-mq: use NOIO context to prevent deadlock during debugfs
creation")):

	/*
	 * debugfs directory and file creation can trigger fs reclaim, which can enter
	 * back into the block layer request_queue. This can cause deadlock if the
	 * queue is frozen. Use NOIO context together with debugfs_mutex to prevent fs
	 * reclaim from triggering block I/O.
	 */

When nvme_alloc_ns() calls nvme_debugfs_register(ns->disk) after
device_add_disk(), the namespace is already on ctrl->namespaces and the
disk is live in sysfs, while blk_register_queue() has already dropped
blk_debugfs_lock(q). If the queue is concurrently frozen (for example, by
a controller reset calling nvme_start_freeze() or a sysfs write calling
blk_mq_freeze_queue()), could GFP_KERNEL allocations inside
debugfs_create_file_aux() trigger direct filesystem reclaim into the
frozen queue and deadlock, or race with concurrent blktrace, scheduler,
rqos, or queue teardown debugfs operations on q->debugfs_dir?

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