Re: RDMA/mlx5: question about a failed destroy in the SRQ create error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/mlx5: untrack on SRQ destroy failure in mlx5_ib_create_srq() leaks state
--

commit f69cb08a14ff31d19dd980f3ad4d401e7db6be8d
Author: Yuho Choi <oss.patchbox@gmail.com>

RDMA/mlx5: question about a failed destroy in the SRQ create error path

This commit proposes untracking a newly created firmware SRQ and leaking its
user buffer and doorbell record when ib_respond_udata() fails in
mlx5_ib_create_srq() and the subsequent mlx5_cmd_destroy_srq() call fails.

> diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
> index 6a4a937d97ce9..ab67ccd9bb845 100644
> --- a/drivers/infiniband/hw/mlx5/srq.c
> +++ b/drivers/infiniband/hw/mlx5/srq.c
> @@ -310,7 +310,15 @@ int mlx5_ib_create_srq(struct ib_srq *ib_srq,
>  	return 0;
>  
>  err_core:
> -	mlx5_cmd_destroy_srq(dev, &srq->msrq);
> +	/*
> +	 * Only user SRQs get here. If a DEVX object still uses the SRQ, the
> +	 * firmware may refuse to destroy it; leak the buffer and doorbell
> +	 * record rather than free pages the device can still access.
> +	 */
> +	if (mlx5_cmd_destroy_srq(dev, &srq->msrq)) {
> +		mlx5_cmd_untrack_srq(dev, &srq->msrq);
> +		return err;
> +	}

[Severity: High]
If mlx5_cmd_destroy_srq() fails here and mlx5_ib_create_srq() untracks the
SRQ and returns err without calling destroy_srq_user(), doesn't that orphan
the live firmware SRQ without any kernel handle to retry destroying it once
the dependent DEVX object is cleaned up, blocking destruction of its parent
objects and leaving a dangling entry on context->db_page_list?

When mlx5_cmd_create_srq() creates the firmware SRQ in mlx5_ib_create_srq(),
the hardware resource is bound to in.pd, in.cqn (either the user CQ or the
device-global kernel CQ dev->devr.c0 for IB_SRQT_BASIC), and in.xrcd (either
the user XRCD or dev->devr.xrcdn0).

When mlx5_ib_create_srq() returns an error to ib_create_srq_user(), the RDMA
core assumes the hardware SRQ was destroyed, decrements usecnt on the parent
objects, and frees srq:

drivers/infiniband/core/verbs.c:ib_create_srq_user() {
	ret = pd->device->ops.create_srq(srq, srq_init_attr, udata);
	if (ret) {
		rdma_restrack_put(&srq->res);
		atomic_dec(&pd->usecnt);
		if (srq->srq_type == IB_SRQT_XRC && srq->ext.xrc.xrcd)
			atomic_dec(&srq->ext.xrc.xrcd->usecnt);
		if (ib_srq_has_cq(srq->srq_type))
			atomic_dec(&srq->ext.cq->usecnt);
		kfree(srq);
		return ERR_PTR(ret);
	}
}

Because the ib_uobject was never initialized with the SRQ and
mlx5_cmd_untrack_srq() removed srq->msrq from dev->srq_table, nothing in the
kernel will ever re-issue the firmware destroy command for srq->msrq.srqn,
even after the dependent DEVX object is destroyed.

As a result, the firmware SRQ stays bound to its PD, CQ, and XRCD forever.
When the user closes the file descriptor, uverbs_destroy_ufile_hw() will try
to destroy those parent objects after cleaning up DEVX objects, which will
fail in firmware and trigger the cleanup warning and forced teardown:

drivers/infiniband/core/uverbs_main.c:uverbs_destroy_ufile_hw() {
	while (!list_empty(&ufile->uobjects) &&
	       !__uverbs_cleanup_ufile(ufile, reason)) {
	}

	if (WARN_ON(!list_empty(&ufile->uobjects)))
		__uverbs_cleanup_ufile(ufile, RDMA_REMOVE_DRIVER_FAILURE);
}

For IB_SRQT_BASIC SRQs, the orphaned firmware SRQ also remains bound to
dev->devr.c0 and dev->devr.xrcdn0, causing ib_destroy_cq(devr->c0) to fail
during device teardown in mlx5_ib_dev_res_cleanup().

In addition, skipping destroy_srq_user() skips mlx5_ib_db_unmap_user() and
ib_umem_release(srq->umem). During create_srq_user(),
mlx5_ib_db_map_user_desc() allocates a struct mlx5_ib_user_db_page, grabs
current->mm, and links page->list onto &context->db_page_list:

drivers/infiniband/hw/mlx5/doorbell.c:mlx5_ib_db_map_user_desc() {
	page->umem = umem;
	if (page->desc.type == IB_UVERBS_BUFFER_TYPE_VA) {
		mmgrab(current->mm);
		page->mm = current->mm;
	}
	list_add(&page->list, &context->db_page_list);

found:
	db->dma = sg_dma_address(page->umem->sgt_append.sgt.sgl) +
		  (desc->addr & ~PAGE_MASK);
	db->u.user_page = page;
	++page->refcnt;
}

Because mlx5_ib_dealloc_ucontext() does not drain context->db_page_list,
skipping mlx5_ib_db_unmap_user() leaves page->list linked to
context->db_page_list after ufile_destroy_ucontext() frees context, while
permanently leaking srq->umem, page->umem, pinned user pages, and the
mmgrab(current->mm) reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028053903.209809-2-oss.patchbox@gmail.com?part=1