Re: [PATCH rdma-next 4/5] RDMA/irdma: Support UMEM attribute for CQ creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rdma/irdma: Memory and resource leak in irdma_resize_cq for 1-step CQs
--

commit fbc0fbebd82d8c5ffc02a6bcacb939600a040cd7
Author: David Hu <xuehaohu@google.com>

RDMA/irdma: Support UMEM attribute for CQ creation

This patch supports the standard 1-step queue buffer creation model for CQs
using UMEM attributes. It sets up hardware PBLs directly from the pinned CQ
and shadow umems, and adds proper cleanup in irdma_cq_free_rsrc() upon CQ
destruction. Legacy 2-step registration is maintained for backward
compatibility.

> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index ab25445f38282..e4a02a6b2a113 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c
> @@ -2028,13 +2028,24 @@ static void irdma_cq_free_rsrc(struct irdma_pci_f *rf, struct irdma_cq *iwcq)
>  		if (iwcq->iwpbl) {
>  			iwmr = iwcq->iwpbl->iwmr;
>  
> -			refcount_dec(&iwmr->user_ring_refs);
> +			if (iwmr && refcount_dec_and_test(&iwmr->user_ring_refs)) {
> +				ib_umem_release(iwmr->region);
> +				if (iwcq->iwpbl->pbl_allocated)
> +					irdma_free_pble(rf->pble_rsrc, &iwcq->iwpbl->pble_alloc);
> +				kfree(iwmr);
> +			}
>  		}

[Severity: High]
Does the introduction of 1-step CQs create a resource leak in 
irdma_resize_cq()?

When userspace resizes the CQ via ib_uverbs_resize_cq(), the kernel invokes 
irdma_resize_cq(). This function successfully swaps in the new PBL and drops 
the reference to the old MR:

drivers/infiniband/hw/irdma/verbs.c:irdma_resize_cq() {
...
	spin_lock_irqsave(&iwcq->lock, flags);
	if (udata) {
		struct irdma_pbl *old_iwpbl = iwcq->iwpbl;
		...
		iwcq->iwpbl = iwpbl_buf;
		if (old_iwpbl) {
			struct irdma_mr *old_iwmr = old_iwpbl->iwmr;

			refcount_dec(&old_iwmr->user_ring_refs);
		}
	}
...
}

For 1-step MRs, the refcount drops from 1 to 0 here. However, since 
irdma_resize_cq() doesn't use refcount_dec_and_test() with the explicit 
cleanup logic added to irdma_cq_free_rsrc(), it appears the MR struct, UMEM 
region, and PBLE resources will be permanently leaked.

If the cleanup logic is added to irdma_resize_cq(), it looks like it would 
cause a sleeping in atomic context bug if placed inside the spin_lock_irqsave 
region, since irdma_free_pble() acquires pble_mutex_lock. The cleanup would 
need to be deferred until after spin_unlock_irqrestore().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011011022.1445352-1-dhu@x6u.co?part=4