Re: [PATCH rdma-next v2 05/13] RDMA/core: extended address handle attributes

[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 AH_EX_CREATE handler
--

commit 4cd859f734837412850e0706a926ea891c66f8a6
Author: Eric Davis <eric.davis@broadcom.com>

RDMA/core: extended address handle attributes

This commit introduces struct rdma_ah_attr_ex to carry extended address
handle attributes such as remote_qpn for UET transports, updates
rdma_create_user_ah() to accept struct rdma_ah_init_attr, and adds the
UVERBS_METHOD_AH_EX_CREATE ioctl method.

> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> index e160786e1df16..27cc1d1622939 100644
> --- a/drivers/infiniband/core/uverbs_std_types.c
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -184,6 +184,73 @@ DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_MW,
>  			    UVERBS_TYPE_ALLOC_IDR(uverbs_free_mw),
>  			    &UVERBS_METHOD(UVERBS_METHOD_MW_DESTROY));
>  
> +/* Creating an address handle is still a write() command whose request
> + * struct cannot grow. A transport (reliable unconnected) that resolves a
> + * peer using both transport and network addresses requires additional
> + * fields in the address handle. Here the address handle create command is
> + * made available as an ioctl method, where additional attributes can be
> + * carried.
> + */
> +static int UVERBS_HANDLER(UVERBS_METHOD_AH_EX_CREATE)(
> +	struct uverbs_attr_bundle *attrs)
> +{
[ ... ]
> +	ah = rdma_create_user_ah(pd, &init_attr, &attrs->driver_udata);
> +	if (IS_ERR(ah))
> +		return PTR_ERR(ah);
> +
> +	ah->uobject = uobj;
> +	uobj->object = ah;
> +
> +	return 0;
> +}

[Severity: High]
Should UVERBS_HANDLER(UVERBS_METHOD_AH_EX_CREATE) call
uverbs_finalize_uobj_create(attrs, UVERBS_ATTR_CREATE_AH_EX_HANDLE) before
returning 0?

Because UVERBS_METHOD_AH_EX_CREATE includes UVERBS_ATTR_UHW(),
ib_uverbs_run_method() can call uverbs_set_output() for UVERBS_ATTR_UHW_OUT
after the handler succeeds:

drivers/infiniband/core/uverbs_ioctl.c:ib_uverbs_run_method() {
	ret = handler(bundle);
	if (ret)
		return ret;

	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, ib_uverbs_cmd_verbs() aborts the
command via bundle_destroy(pbundle, false) -> uverbs_finalize_object() ->
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 uverbs_finalize_uobj_create() setting the bit in
pbundle->uobj_hw_obj_valid, hw_obj_valid is false on abort, so destroy_hw
(uverbs_free_ah() -> rdma_destroy_ah_user()) is skipped. Does this leak the
newly created struct ib_ah, its ah->sgid_attr reference, and its pd->usecnt
reference?

[ ... ]

> +DECLARE_UVERBS_NAMED_METHOD(
> +	UVERBS_METHOD_AH_EX_CREATE,
> +	UVERBS_ATTR_IDR(UVERBS_ATTR_CREATE_AH_EX_HANDLE,
> +			UVERBS_OBJECT_AH,
> +			UVERBS_ACCESS_NEW,
> +			UA_MANDATORY),
> +	UVERBS_ATTR_IDR(UVERBS_ATTR_CREATE_AH_EX_PD_HANDLE,
> +			UVERBS_OBJECT_PD,
> +			UVERBS_ACCESS_READ,
> +			UA_MANDATORY),
> +	UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_AH_EX_AH_ATTR_EX,
> +			   UVERBS_ATTR_STRUCT(struct ib_uverbs_ah_attr_ex,
> +					      remote_qpn),
> +			   UA_MANDATORY),
> +	UVERBS_ATTR_UHW());

[ ... ]

> -- 
> 2.51.2
>

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