[PATCH rdma-next v2] RDMA/irdma: Check MR contiguity before PBLE alloc

[PATCH rdma-next v2] RDMA/irdma: Check MR contiguity before PBLE alloc

[Jacob Moroni]

The original logic would attempt to allocate PBLEs, then would
check whether the region was contiguous _after_, and if so,
would free the PBLEs and use a direct mapping instead.

These unnecessary temporary PBLE allocations can technically
fail (PBLEs are a finite resource), which can cause an MR
registration failure even if the region is contiguous and
should have succeeded.

Fix this by checking whether the region is contiguous before
allocating any PBLEs. This also provides an opportunity to
consolidate some of the rereg_mr logic and use the existing
ib_umem_is_contiguous() helper.

Tested on GEN3 HW with a test program that does an extensive
sweep of MR size/alignment/hugepage combinations, but most
importantly, first/last byte reachable and -1/+1 unreachable
when using contig regions with both aligned and unaligned start
addresses, plus a kprobe on irdma_get_pble() to ensure we're
actually hitting the new path.

Ring MRs still have their own logic. That will be addressed next.

Signed-off-by: Jacob Moroni <jmoroni@google.com>
---
Changes in v2:
- Skip the ib_umem_find_best_pgsz() call if we already know the
  region is contiguous (Jason)
- Move MEM MR page size/PBLE handling into irdma_reg_user_mr_type_mem()
  and drop the is_contig flag added in v1
- Drop the now-unused pgaddrmem[0] store in irdma_setup_pbles()
- Add test description

 drivers/infiniband/hw/irdma/verbs.c | 97 +++++++++--------------------
 1 file changed, 29 insertions(+), 68 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index 71d7ae7baa47..f89c8516c57c 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -2917,42 +2917,6 @@ static bool irdma_check_mem_contiguous(u64 *arr, u32 npages, u32 pg_size)
 	return true;
 }
 
-/**
- * irdma_check_mr_contiguous - check if MR is physically contiguous
- * @palloc: pbl allocation struct
- * @pg_size: page size
- */
-static bool irdma_check_mr_contiguous(struct irdma_pble_alloc *palloc,
-				      u32 pg_size)
-{
-	struct irdma_pble_level2 *lvl2 = &palloc->level2;
-	struct irdma_pble_info *leaf = lvl2->leaf;
-	u64 *arr = NULL;
-	u64 *start_addr = NULL;
-	int i;
-	bool ret;
-
-	if (palloc->level == PBLE_LEVEL_1) {
-		arr = palloc->level1.addr;
-		ret = irdma_check_mem_contiguous(arr, palloc->total_cnt,
-						 pg_size);
-		return ret;
-	}
-
-	start_addr = leaf->addr;
-
-	for (i = 0; i < lvl2->leaf_cnt; i++, leaf++) {
-		arr = leaf->addr;
-		if ((*start_addr + ((u64)i * pg_size * PBLE_PER_PAGE)) != *arr)
-			return false;
-		ret = irdma_check_mem_contiguous(arr, leaf->cnt, pg_size);
-		if (!ret)
-			return false;
-	}
-
-	return true;
-}
-
 /**
  * irdma_setup_pbles - copy user pg address to pble's
  * @rf: RDMA PCI function
@@ -2989,9 +2953,6 @@ static int irdma_setup_pbles(struct irdma_pci_f *rf, struct irdma_mr *iwmr,
 
 	irdma_copy_user_pgaddrs(iwmr, pbl, pbl_len, level);
 
-	if (lvl)
-		iwmr->pgaddrmem[0] = *pbl;
-
 	return 0;
 }
 
@@ -3413,24 +3374,33 @@ static int irdma_reg_user_mr_type_mem(struct irdma_mr *iwmr, int access,
 {
 	struct irdma_device *iwdev = to_iwdev(iwmr->ibmr.device);
 	struct irdma_pbl *iwpbl = &iwmr->iwpbl;
+	struct ib_umem *region = iwmr->region;
 	u32 stag = 0;
-	u8 lvl;
 	int err;
 
-	lvl = iwmr->page_cnt != 1 ? PBLE_LEVEL_1 | PBLE_LEVEL_2 : PBLE_LEVEL_0;
 	iwmr->access = access;
 
-	err = irdma_setup_pbles(iwdev->rf, iwmr, lvl);
-	if (err)
-		return err;
-
-	if (lvl) {
-		err = irdma_check_mr_contiguous(&iwpbl->pble_alloc,
+	if (ib_umem_is_contiguous(region)) {
+		/* HW needs a valid page size even without PBLEs, so use 4K. It
+		 * behaves like a PBLE-backed region where the PBA must be page
+		 * aligned. HW takes the page offset from the lower 12 VA bits.
+		 */
+		iwmr->page_size = SZ_4K;
+		iwmr->pgaddrmem[0] = ALIGN_DOWN(ib_umem_start_dma_addr(region),
 						iwmr->page_size);
-		if (err) {
-			irdma_free_pble(iwdev->rf->pble_rsrc, &iwpbl->pble_alloc);
-			iwpbl->pbl_allocated = false;
-		}
+	} else {
+		iwmr->page_size = ib_umem_find_best_pgsz(region,
+				iwdev->rf->sc_dev.hw_attrs.page_size_cap,
+				iwmr->ibmr.iova);
+		if (unlikely(!iwmr->page_size))
+			return -EOPNOTSUPP;
+
+		iwmr->page_cnt = ib_umem_num_dma_blocks(region,
+							iwmr->page_size);
+		err = irdma_setup_pbles(iwdev->rf, iwmr,
+					PBLE_LEVEL_1 | PBLE_LEVEL_2);
+		if (err)
+			return err;
 	}
 
 	if (create_stag) {
@@ -3466,10 +3436,8 @@ static struct irdma_mr *irdma_alloc_iwmr(struct ib_umem *region,
 					 struct ib_pd *pd, u64 virt,
 					 enum irdma_memreg_type reg_type)
 {
-	struct irdma_device *iwdev = to_iwdev(pd->device);
 	struct irdma_pbl *iwpbl;
 	struct irdma_mr *iwmr;
-	unsigned long pgsz_bitmap;
 
 	iwmr = kzalloc_obj(*iwmr);
 	if (!iwmr)
@@ -3483,18 +3451,21 @@ static struct irdma_mr *irdma_alloc_iwmr(struct ib_umem *region,
 	iwmr->ibmr.device = pd->device;
 	iwmr->ibmr.iova = virt;
 	iwmr->type = reg_type;
+	iwmr->len = region->length;
+	iwpbl->user_base = virt;
 
-	pgsz_bitmap = (reg_type == IRDMA_MEMREG_TYPE_MEM) ?
-		iwdev->rf->sc_dev.hw_attrs.page_size_cap : SZ_4K;
+	/* Page size and count are calculated in irdma_reg_user_mr_type_mem()
+	 * for MEM type MRs.
+	 */
+	if (reg_type == IRDMA_MEMREG_TYPE_MEM)
+		return iwmr;
 
-	iwmr->page_size = ib_umem_find_best_pgsz(region, pgsz_bitmap, virt);
+	iwmr->page_size = ib_umem_find_best_pgsz(region, SZ_4K, virt);
 	if (unlikely(!iwmr->page_size)) {
 		kfree(iwmr);
 		return ERR_PTR(-EOPNOTSUPP);
 	}
 
-	iwmr->len = region->length;
-	iwpbl->user_base = virt;
 	iwmr->page_cnt = ib_umem_num_dma_blocks(region, iwmr->page_size);
 
 	return iwmr;
@@ -3833,7 +3804,6 @@ static int irdma_hwdereg_mr(struct ib_mr *ib_mr)
 static int irdma_rereg_mr_trans(struct irdma_mr *iwmr, u64 start, u64 len,
 				u64 virt)
 {
-	struct irdma_device *iwdev = to_iwdev(iwmr->ibmr.device);
 	struct irdma_pbl *iwpbl = &iwmr->iwpbl;
 	struct ib_pd *pd = iwmr->ibmr.pd;
 	struct ib_umem *region;
@@ -3846,17 +3816,8 @@ static int irdma_rereg_mr_trans(struct irdma_mr *iwmr, u64 start, u64 len,
 	iwmr->region = region;
 	iwmr->ibmr.iova = virt;
 	iwmr->ibmr.pd = pd;
-	iwmr->page_size = ib_umem_find_best_pgsz(region,
-				iwdev->rf->sc_dev.hw_attrs.page_size_cap,
-				virt);
-	if (unlikely(!iwmr->page_size)) {
-		err = -EOPNOTSUPP;
-		goto err;
-	}
-
 	iwmr->len = region->length;
 	iwpbl->user_base = virt;
-	iwmr->page_cnt = ib_umem_num_dma_blocks(region, iwmr->page_size);
 
 	err = irdma_reg_user_mr_type_mem(iwmr, iwmr->access, false);
 	if (err)
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH rdma-next v2] RDMA/irdma: Check MR contiguity before PBLE alloc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] irdma: double free of PBLE on MR reregistration HW failure
--

commit 448b999143bf40a6d4795a1e901ceab0ac8c8c2d
Author: Jacob Moroni <jmoroni@google.com>

RDMA/irdma: Check MR contiguity before PBLE alloc

This commit checks if a memory region is contiguous before attempting to
allocate PBLEs. This prevents unnecessary PBLE allocation failures from
causing memory registration to fail when a direct mapping could have been
used instead.

> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index 71d7ae7baa478..f89c8516c57c0 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c

[ ... ]

> @@ -3413,24 +3374,33 @@ static int irdma_reg_user_mr_type_mem(struct irdma_mr *iwmr, int access,
>  {
>  	struct irdma_device *iwdev = to_iwdev(iwmr->ibmr.device);
>  	struct irdma_pbl *iwpbl = &iwmr->iwpbl;
> +	struct ib_umem *region = iwmr->region;
>  	u32 stag = 0;
> -	u8 lvl;
>  	int err;
>  
> -	lvl = iwmr->page_cnt != 1 ? PBLE_LEVEL_1 | PBLE_LEVEL_2 : PBLE_LEVEL_0;
>  	iwmr->access = access;
>  
> -	err = irdma_setup_pbles(iwdev->rf, iwmr, lvl);
> -	if (err)
> -		return err;
> -
> -	if (lvl) {
> -		err = irdma_check_mr_contiguous(&iwpbl->pble_alloc,
> +	if (ib_umem_is_contiguous(region)) {
> +		/* HW needs a valid page size even without PBLEs, so use 4K. It
> +		 * behaves like a PBLE-backed region where the PBA must be page
> +		 * aligned. HW takes the page offset from the lower 12 VA bits.
> +		 */
> +		iwmr->page_size = SZ_4K;
> +		iwmr->pgaddrmem[0] = ALIGN_DOWN(ib_umem_start_dma_addr(region),
>  						iwmr->page_size);
> -		if (err) {
> -			irdma_free_pble(iwdev->rf->pble_rsrc, &iwpbl->pble_alloc);
> -			iwpbl->pbl_allocated = false;
> -		}
> +	} else {
> +		iwmr->page_size = ib_umem_find_best_pgsz(region,
> +				iwdev->rf->sc_dev.hw_attrs.page_size_cap,
> +				iwmr->ibmr.iova);
> +		if (unlikely(!iwmr->page_size))
> +			return -EOPNOTSUPP;
> +
> +		iwmr->page_cnt = ib_umem_num_dma_blocks(region,
> +							iwmr->page_size);
> +		err = irdma_setup_pbles(iwdev->rf, iwmr,
> +					PBLE_LEVEL_1 | PBLE_LEVEL_2);
> +		if (err)
> +			return err;
>  	}
>  
>  	if (create_stag) {
>  		stag = irdma_create_stag(iwdev);
>  		if (!stag) {
>  			err = -ENOMEM;
>  			goto free_pble;
>  		}
>  
>  		iwmr->stag = stag;
>  		iwmr->ibmr.rkey = stag;
>  		iwmr->ibmr.lkey = stag;
>  	}
>  
>  	err = irdma_hwreg_mr(iwdev, iwmr, access);
>  	if (err)
>  		goto err_hwreg;
>  
>  	return 0;
>  
>  err_hwreg:
>  	if (stag)
>  		irdma_free_stag(iwdev, stag);
>  
>  free_pble:
>  	if (iwpbl->pble_alloc.level != PBLE_LEVEL_0 && iwpbl->pbl_allocated)
>  		irdma_free_pble(iwdev->rf->pble_rsrc, &iwpbl->pble_alloc);

[Severity: High]
If irdma_hwreg_mr() fails during memory registration, the free_pble error
path frees the PBLE allocator resources but does not clear
iwpbl->pbl_allocated.

When this occurs during MR reregistration in irdma_rereg_mr_trans(), the
error propagates up to the RDMA core, which retains the original MR.

Upon subsequent destruction of the MR in irdma_dereg_mr(), the driver
checks this stale flag:

	if (iwpbl->pbl_allocated)
		irdma_free_pble(iwdev->rf->pble_rsrc, &iwpbl->pble_alloc);

Could this double free corrupt the driver's internal memory management data
structures (irdma_prm_return_pbles), leading to kernel memory corruption?

>  
>  	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014162144.4053279-1-jmoroni@google.com?part=1