Re: [PATCH rdma-next v2 07/13] RDMA/core: job and job key objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/uverbs: missing uverbs_finalize_uobj_create() in job alloc and import
--

commit 5f0d9f9de261cb667b880d34e0c2d952d5a61944
Author: Eric Davis <eric.davis@broadcom.com>

RDMA/core: job and job key objects

This commit introduces job and job key objects to RDMA core to support
unconnected transports that demultiplex incoming traffic on a JobID. It
also allows jobs to be exported to a file descriptor and imported into
another context.

> diff --git a/drivers/infiniband/core/uverbs_std_types_job.c b/drivers/infiniband/core/uverbs_std_types_job.c
> new file mode 100644
> index 0000000000000..20acb2aa9871f
> --- /dev/null
> +++ b/drivers/infiniband/core/uverbs_std_types_job.c

[ ... ]

> +static int UVERBS_HANDLER(UVERBS_METHOD_JOB_IMPORT)(
> +	struct uverbs_attr_bundle *attrs)
> +{
> +	struct ib_uobject *uobj =
> +		uverbs_attr_get_uobject(attrs, UVERBS_ATTR_IMPORT_JOB_HANDLE);
> +	struct ib_device *ib_dev = attrs->context->device;
> +	struct ib_job *job;
> +	int fd, ret;
> +
> +	ret = uverbs_get_raw_fd(&fd, attrs, UVERBS_ATTR_IMPORT_JOB_FD);
> +	if (ret)
> +		return ret;
> +
> +	ret = ib_job_get_from_fd(fd, ib_dev, &job);
> +	if (ret)
> +		return ret;
> +
> +	uobj->object = job;
> +
> +	return 0;
> +}
> +
> +static int uverbs_free_job(struct ib_uobject *uobject,
> +			   enum rdma_remove_reason why,
> +			   struct uverbs_attr_bundle *attrs)
> +{
> +	struct ib_ujob_object *ujob =
> +		container_of(uobject, struct ib_ujob_object, uobject);
> +	struct ib_job *job = uobject->object;
> +
> +	/* don't free if there are still references to this job */
> +	if (atomic_read(&ujob->refcnt))
> +		return -EBUSY;
> +
> +	kref_put(&job->ref, ib_job_release);
> +
> +	return 0;
> +}
> +
> +static int UVERBS_HANDLER(UVERBS_METHOD_JOB_ALLOC)(
> +	struct uverbs_attr_bundle *attrs)
> +{
> +	struct ib_uobject *uobj =
> +		uverbs_attr_get_uobject(attrs, UVERBS_ATTR_ALLOC_JOB_HANDLE);
> +	struct ib_device *ib_dev = attrs->context->device;
> +	struct ib_job_attr attr = {};
> +	struct ib_job *job;
> +	int ret;

[ ... ]

> +	job = rdma_zalloc_drv_obj(ib_dev, ib_job);
> +	if (!job)
> +		return -ENOMEM;
> +
> +	job->device = ib_dev;
> +	job->job_id = attr.job_id;
> +	job->max_addr_entries = attr.max_addr_entries;
> +	kref_init(&job->ref);
> +	uobj->object = job;
> +
> +	rdma_restrack_new(&job->res, RDMA_RESTRACK_JOB);
> +	rdma_restrack_set_name(&job->res, NULL);
> +
> +	ret = ib_dev->ops.alloc_job(job, &attr, attrs);
> +	if (ret)
> +		goto err_free;
> +
> +	rdma_restrack_add(&job->res);
> +
> +	return 0;

[Severity: High]
Should UVERBS_HANDLER(UVERBS_METHOD_JOB_ALLOC) and
UVERBS_HANDLER(UVERBS_METHOD_JOB_IMPORT) call uverbs_finalize_uobj_create()
before returning 0?

UVERBS_METHOD_JOB_ALLOC declares UVERBS_ATTR_UHW(), which sets
bundle->method_elm->has_udata = true. After the handler returns 0,
ib_uverbs_run_method() updates UVERBS_ATTR_UHW_OUT when present:

drivers/infiniband/core/uverbs_ioctl.c:ib_uverbs_run_method() {
	...
	ret = handler(&pbundle->bundle);
	if (!ret && bundle->method_elm->has_udata) {
		const struct uverbs_attr *attr =
			uverbs_attr_get(bundle, UVERBS_ATTR_UHW_OUT);

		if (!IS_ERR(attr))
			ret = uverbs_set_output(bundle, attr);
	}
	...
}

If uverbs_set_output() fails with -EFAULT when writing to user_attrs,
ib_uverbs_cmd_verbs() calls bundle_destroy(pbundle, false). Because
uverbs_finalize_uobj_create() was not called to mark uobj_hw_obj_valid,
uverbs_finalize_object() passes hw_obj_valid = false to
rdma_alloc_abort_uobject():

drivers/infiniband/core/rdma_core.c:rdma_alloc_abort_uobject() {
	...
	if (hw_obj_valid) {
		uverbs_get_cleared_udata(attrs);
		ret = uobj->uapi_object->type_class->destroy_hw(
			uobj, RDMA_REMOVE_ABORT, attrs);
	...
}

Without hw_obj_valid set, destroy_hw (uverbs_free_job()) is skipped on the
abort path. Does this leak the struct ib_job allocation, the driver job
resource from alloc_job(), and leave job->res in the RDMA_RESTRACK_JOB
table?

[ ... ]

> +DECLARE_UVERBS_NAMED_METHOD(
> +	UVERBS_METHOD_JOB_ALLOC,
> +	UVERBS_ATTR_IDR(UVERBS_ATTR_ALLOC_JOB_HANDLE,
> +			UVERBS_OBJECT_JOB,
> +			UVERBS_ACCESS_NEW,
> +			UA_MANDATORY),
> +	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_ALLOC_JOB_ID,
> +			   UVERBS_ATTR_TYPE(u32),
> +			   UA_MANDATORY),
> +	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_ALLOC_JOB_MAX_ADDR_ENTRIES,
> +			   UVERBS_ATTR_TYPE(u32),
> +			   UA_MANDATORY),
> +	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_ALLOC_JOB_PORT_NUM,
> +			   UVERBS_ATTR_TYPE(u32),
> +			   UA_OPTIONAL),
> +	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_ALLOC_JOB_SGID_INDEX,
> +			   UVERBS_ATTR_TYPE(u8),
> +			   UA_OPTIONAL),
> +	UVERBS_ATTR_UHW());

[ ... ]

完美结束，确保最后有一个空行。

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-uet-ib-core-v2-0-8bd654f25370@broadcom.com?part=7