Re: [PATCH v7 1/2] nvme: add per-queue sysfs directories

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvme: missing kobject_del() on error path in nvme_add_queue_info()
--

commit 6e5f76664582ac2e5b614fa65fb1066286cd6645
Author: Saravanan D <saravanand@crusoe.ai>

nvme: add per-queue sysfs directories

This commit adds helpers to create a per-controller queues sysfs directory
and manage per-queue subdirectories (/sys/class/nvme/nvmeX/queues/<qid>/)
populated with transport-provided attribute groups.

Link: https://lore.kernel.org/linux-nvme/13c160d0-0657-47ce-9a16-58d348267ab0@grimberg.me/

> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 10018c6356b62..8a982453cb69d 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c

[ ... ]

> +struct nvme_queue_info *
> +nvme_add_queue_info(struct nvme_ctrl *ctrl, unsigned int qid, void *data)
> +{
> +	struct nvme_queue_info *qinfo;
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!ctrl->queues_kobj))
> +		return ERR_PTR(-EINVAL);
> +
> +	qinfo = kzalloc_obj(*qinfo);
> +	if (!qinfo)
> +		return ERR_PTR(-ENOMEM);
> +	qinfo->data = data;
> +
> +	ret = kobject_init_and_add(&qinfo->kobj, &nvme_queue_info_ktype,
> +				   ctrl->queues_kobj, "%u", qid);
> +	if (ret)
> +		goto out_put;
> +
> +	ret = sysfs_create_groups(&qinfo->kobj, ctrl->ops->queue_attr_groups);
> +	if (ret)
> +		goto out_put;
> +	return qinfo;
> +
> +out_put:
> +	kobject_put(&qinfo->kobj);
> +	return ERR_PTR(ret);
> +}

[Severity: Medium]
If sysfs_create_groups() fails in nvme_add_queue_info() after
kobject_init_and_add() has already published qinfo->kobj in sysfs, should
the error path call kobject_del(&qinfo->kobj) before kobject_put(&qinfo->kobj),
just like nvme_remove_queue_info() does?

Without kobject_del(), unlinking the directory from sysfs is deferred to
kobject_cleanup(). If CONFIG_DEBUG_KOBJECT_RELEASE is enabled or if another
thread grabbed a reference to qinfo->kobj after it was added to sysfs, the
<qid> directory and its parent reference on ctrl->queues_kobj remain live
after nvme_add_queue_info() returns an error, which can cause a subsequent
retry or reconnect for the same qid to fail with -EEXIST or expose a
dangling qinfo->data pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793800758.git.saravanand@crusoe.ai?part=1