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

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

[Yuho Choi]

Hi Leon,

I have a question about the error path of mlx5_ib_create_srq() (rdma
for-next, c3e8e9451e6f) and would appreciate your view. When copying
the response to user space fails, the result of the destroy is not
checked:

	err_core:
		mlx5_cmd_destroy_srq(dev, &srq->msrq);

	err_usr_kern_srq:
		if (udata)
			destroy_srq_user(ib_srq->pd, srq, udata);

If I read it correctly, a failed destroy here would leave the firmware
SRQ using the SRQ buffer and doorbell record that are then released.
Since fd89099d635e ("RDMA/mlx5: Issue FW command to destroy SRQ on
reentry") the SRQ would also be back in dev->srq_table after the core
frees the ib_srq, so SRQ events and ODP page faults (odp_get_rsc())
could find freed memory.

Thanks for explaining in [1] that DESTROY of kernel-owned objects does
not fail on working firmware. I wondered whether user SRQs might be
different, since they carry the uid of their context and a DEVX QP of
the same uid can use them, which 43d781b9fa56 ("RDMA: Allow fail of
destroy CQ") mentions as a reason for destroy to fail. For example,
could user space create such a DEVX QP on the new SRQ number (probing
for it) while the response copy is held up, say on a FUSE-backed
buffer, and then let the copy fail? I could not tell from the tree
whether the firmware would refuse the destroy in that case, so I may
well be missing something.

If this is a real problem, would something like the untested diff below
be acceptable? It keeps the memory and drops the table entry, at the
cost of leaking the SRQ. If you prefer, such SRQs could instead be kept
on the ucontext and destroyed again from mlx5_ib_dealloc_ucontext(),
which avoids the leak.

This was found with a static analysis tool from ongoing, not yet
published research, and confirmed by code inspection. It has not been
reproduced, and I used an LLM to help with the analysis and this mail.

[1] https://lore.kernel.org/r/20261016045849.813696-6-oss.patchbox@gmail.com

Thanks,
Yuho

diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index 6a4a937d97ce..96ea15ba0487 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;
+	}

 err_usr_kern_srq:
 	if (udata)
diff --git a/drivers/infiniband/hw/mlx5/srq.h b/drivers/infiniband/hw/mlx5/srq.h
index a7e3dc5564ac..4cd63e8d8579 100644
--- a/drivers/infiniband/hw/mlx5/srq.h
+++ b/drivers/infiniband/hw/mlx5/srq.h
@@ -58,6 +58,7 @@ struct mlx5_srq_table {
 int mlx5_cmd_create_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq,
 			struct mlx5_srq_attr *in);
 int mlx5_cmd_destroy_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq);
+void mlx5_cmd_untrack_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq);
 int mlx5_cmd_query_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq,
 		       struct mlx5_srq_attr *out);
 int mlx5_cmd_arm_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq,
diff --git a/drivers/infiniband/hw/mlx5/srq_cmd.c b/drivers/infiniband/hw/mlx5/srq_cmd.c
index c1a088120915..c97e22946ef3 100644
--- a/drivers/infiniband/hw/mlx5/srq_cmd.c
+++ b/drivers/infiniband/hw/mlx5/srq_cmd.c
@@ -697,6 +697,22 @@ int mlx5_cmd_destroy_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq)
 	return 0;
 }

+/*
+ * For an SRQ whose destroy failed in the error flow of its creation: the
+ * firmware object stays, but the caller is about to free @srq, which
+ * mlx5_cmd_destroy_srq() put back into the table. Remove it and wait for
+ * concurrent lookups to drop their references.
+ */
+void mlx5_cmd_untrack_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq)
+{
+	struct mlx5_srq_table *table = &dev->srq_table;
+
+	xa_cmpxchg_irq(&table->array, srq->srqn, srq, NULL, 0);
+
+	mlx5_core_res_put(&srq->common);
+	wait_for_completion(&srq->common.free);
+}
+
 int mlx5_cmd_query_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq,
 		       struct mlx5_srq_attr *out)
 {

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

[sashiko-bot]

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