[PATCH RESEND rdma-next 1/2] RDMA/irdma: Do not allow rereg_mr to revive defunct MRs

[PATCH RESEND rdma-next 1/2] RDMA/irdma: Do not allow rereg_mr to revive defunct MRs

[Jacob Moroni]

An MR can become "defunct" if a previous rereg_mr happened
to fail in between deregistering the old MR in HW and
reregistering it (e.g., due to ib_umem_get_va() failing
in the IB_MR_REREG_TRANS case).

In this case, the MR is left in a state where the umem may
have already been released.

If rereg_mr then runs again, but without IB_MR_REREG_TRANS,
it will end up reregistering the MR with stale translation
info.

Fix this by rejecting any attempt to reregister an MR that
has previously failed during rereg_mr after the point where
the MR has been deregistered in HW.

Fixes: 5ac388db27c4 ("RDMA/irdma: Add support to re-register a memory region")
Signed-off-by: Jacob Moroni <jmoroni@google.com>
---
 drivers/infiniband/hw/irdma/verbs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index c91c55917de9..fd1b1454f026 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -3873,6 +3873,9 @@ static struct ib_mr *irdma_rereg_user_mr(struct ib_mr *ib_mr, int flags,
 	if (ret)
 		return ERR_PTR(ret);
 
+	if (!iwmr->is_hwreg)
+		return ERR_PTR(-EINVAL);
+
 	ret = irdma_hwdereg_mr(ib_mr);
 	if (ret)
 		return ERR_PTR(ret);

base-commit: c3e8e9451e6f1b69f685b75e0fbf14f3d0fb2539
prerequisite-patch-id: 6e527df00a7bd3c3941ab1af65ab5d7115264bb6
-- 
2.56.0.313.g2479322e68-goog

[PATCH RESEND rdma-next 2/2] RDMA/irdma: Do not change mr->pd in driver during rereg_mr

[Jacob Moroni]

Previously, IB_MR_REREG_PD would cause the driver to set
iwmr->ibmr.pd to the new PD before reregistering with HW because
irdma_hwreg_mr() retrieves the PD from there.

ib_uverbs_rereg_mr() only moves the PD usecnt from the old PD to
the new one and updates mr->pd itself after the driver's
rereg_user_mr() call succeeds.

So, if the rereg_user_mr() call fails, the MR is left pointing at
a PD that does not account for it while the old PD keeps the reference,
which means the old PD can never be deallocated, while the new PD can
be deallocated even though the MR still points at it.

Fix this by removing the PD update from the driver and just letting
the core handle it. This required plumbing the PD as an argument
through a few functions. This works because the core passes the
PD unconditionally and provides the current PD if IB_MR_REREG_PD is
not set.

Fixes: 5ac388db27c4 ("RDMA/irdma: Add support to re-register a memory region")
Signed-off-by: Jacob Moroni <jmoroni@google.com>
---
 drivers/infiniband/hw/irdma/verbs.c | 34 ++++++++++++-----------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index fd1b1454f026..b4343181159d 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -3308,14 +3308,14 @@ static int irdma_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
  * irdma_hwreg_mr - send cqp command for memory registration
  * @iwdev: irdma device
  * @iwmr: irdma mr pointer
+ * @pd: protection domain to register the MR with
  * @access: access for MR
  */
 static int irdma_hwreg_mr(struct irdma_device *iwdev, struct irdma_mr *iwmr,
-			  u16 access)
+			  struct ib_pd *pd, u16 access)
 {
 	struct irdma_pbl *iwpbl = &iwmr->iwpbl;
 	struct irdma_reg_ns_stag_info *stag_info;
-	struct ib_pd *pd = iwmr->ibmr.pd;
 	struct irdma_pd *iwpd = to_iwpd(pd);
 	struct irdma_pble_alloc *palloc = &iwpbl->pble_alloc;
 	struct irdma_cqp_request *cqp_request;
@@ -3369,8 +3369,8 @@ static int irdma_hwreg_mr(struct irdma_device *iwdev, struct irdma_mr *iwmr,
 	return ret;
 }
 
-static int irdma_reg_user_mr_type_mem(struct irdma_mr *iwmr, int access,
-				      bool create_stag)
+static int irdma_reg_user_mr_type_mem(struct irdma_mr *iwmr, struct ib_pd *pd,
+				      int access, bool create_stag)
 {
 	struct irdma_device *iwdev = to_iwdev(iwmr->ibmr.device);
 	struct irdma_pbl *iwpbl = &iwmr->iwpbl;
@@ -3415,7 +3415,7 @@ static int irdma_reg_user_mr_type_mem(struct irdma_mr *iwmr, int access,
 		iwmr->ibmr.lkey = stag;
 	}
 
-	err = irdma_hwreg_mr(iwdev, iwmr, access);
+	err = irdma_hwreg_mr(iwdev, iwmr, pd, access);
 	if (err)
 		goto err_hwreg;
 
@@ -3646,7 +3646,7 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
 			goto error;
 		break;
 	case IRDMA_MEMREG_TYPE_MEM:
-		err = irdma_reg_user_mr_type_mem(iwmr, access, true);
+		err = irdma_reg_user_mr_type_mem(iwmr, pd, access, true);
 		if (err)
 			goto error;
 
@@ -3726,7 +3726,7 @@ static struct ib_mr *irdma_reg_user_mr_dmabuf(struct ib_pd *pd, u64 start,
 		goto err_release;
 	}
 
-	err = irdma_reg_user_mr_type_mem(iwmr, access, true);
+	err = irdma_reg_user_mr_type_mem(iwmr, pd, access, true);
 	if (err)
 		goto err_iwmr;
 
@@ -3794,6 +3794,7 @@ static int irdma_hwdereg_mr(struct ib_mr *ib_mr)
 /*
  * irdma_rereg_mr_trans - Re-register a user MR for a change translation.
  * @iwmr: ptr of iwmr
+ * @pd: protection domain to register the MR with
  * @start: virtual start address
  * @len: length of mr
  * @virt: virtual address
@@ -3801,11 +3802,10 @@ static int irdma_hwdereg_mr(struct ib_mr *ib_mr)
  * Re-register a user memory region when a change translation is requested.
  * Re-register a new region while reusing the stag from the original registration.
  */
-static int irdma_rereg_mr_trans(struct irdma_mr *iwmr, u64 start, u64 len,
-				u64 virt)
+static int irdma_rereg_mr_trans(struct irdma_mr *iwmr, struct ib_pd *pd,
+				u64 start, u64 len, u64 virt)
 {
 	struct irdma_pbl *iwpbl = &iwmr->iwpbl;
-	struct ib_pd *pd = iwmr->ibmr.pd;
 	struct ib_umem *region;
 	int err;
 
@@ -3815,11 +3815,10 @@ 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->len = region->length;
 	iwpbl->user_base = virt;
 
-	err = irdma_reg_user_mr_type_mem(iwmr, iwmr->access, false);
+	err = irdma_reg_user_mr_type_mem(iwmr, pd, iwmr->access, false);
 	if (err)
 		goto err;
 
@@ -3883,11 +3882,6 @@ static struct ib_mr *irdma_rereg_user_mr(struct ib_mr *ib_mr, int flags,
 	if (flags & IB_MR_REREG_ACCESS)
 		iwmr->access = new_access;
 
-	if (flags & IB_MR_REREG_PD) {
-		iwmr->ibmr.pd = new_pd;
-		iwmr->ibmr.device = new_pd->device;
-	}
-
 	if (flags & IB_MR_REREG_TRANS) {
 		if (iwpbl->pbl_allocated) {
 			irdma_free_pble(iwdev->rf->pble_rsrc,
@@ -3900,9 +3894,9 @@ static struct ib_mr *irdma_rereg_user_mr(struct ib_mr *ib_mr, int flags,
 			iwmr->region = NULL;
 		}
 
-		ret = irdma_rereg_mr_trans(iwmr, start, len, virt);
+		ret = irdma_rereg_mr_trans(iwmr, new_pd, start, len, virt);
 	} else {
-		ret = irdma_hwreg_mr(iwdev, iwmr, iwmr->access);
+		ret = irdma_hwreg_mr(iwdev, iwmr, new_pd, iwmr->access);
 	}
 
 	return ret ? ERR_PTR(ret) : NULL;
@@ -3951,7 +3945,7 @@ struct ib_mr *irdma_reg_phys_mr(struct ib_pd *pd, u64 addr, u64 size, int access
 	iwmr->pgaddrmem[0] = addr;
 	iwmr->len = size;
 	iwmr->page_size = SZ_4K;
-	ret = irdma_hwreg_mr(iwdev, iwmr, access);
+	ret = irdma_hwreg_mr(iwdev, iwmr, pd, access);
 	if (ret) {
 		irdma_free_stag(iwdev, stag);
 		goto err;
-- 
2.56.0.313.g2479322e68-goog

Re: [PATCH RESEND rdma-next 1/2] RDMA/irdma: Do not allow rereg_mr to revive defunct MRs

[sashiko-bot]

> An MR can become "defunct" if a previous rereg_mr happened
> to fail in between deregistering the old MR in HW and
> reregistering it (e.g., due to ib_umem_get_va() failing
> in the IB_MR_REREG_TRANS case).
> 
> In this case, the MR is left in a state where the umem may
> have already been released.
> 
> If rereg_mr then runs again, but without IB_MR_REREG_TRANS,
> it will end up reregistering the MR with stale translation
> info.
> 
> Fix this by rejecting any attempt to reregister an MR that
> has previously failed during rereg_mr after the point where
> the MR has been deregistered in HW.
> 
> Fixes: 5ac388db27c4 ("RDMA/irdma: Add support to re-register a memory region")
> Signed-off-by: Jacob Moroni <jmoroni@google.com>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH RESEND rdma-next 2/2] RDMA/irdma: Do not change mr->pd in driver during rereg_mr

[sashiko-bot]

> Previously, IB_MR_REREG_PD would cause the driver to set
> iwmr->ibmr.pd to the new PD before reregistering with HW because
> irdma_hwreg_mr() retrieves the PD from there.
> 
> ib_uverbs_rereg_mr() only moves the PD usecnt from the old PD to
> the new one and updates mr->pd itself after the driver's
> rereg_user_mr() call succeeds.
> 
> So, if the rereg_user_mr() call fails, the MR is left pointing at
> a PD that does not account for it while the old PD keeps the reference,
> which means the old PD can never be deallocated, while the new PD can
> be deallocated even though the MR still points at it.
> 
> Fix this by removing the PD update from the driver and just letting
> the core handle it. This required plumbing the PD as an argument
> through a few functions. This works because the core passes the
> PD unconditionally and provides the current PD if IB_MR_REREG_PD is
> not set.
> 
> Fixes: 5ac388db27c4 ("RDMA/irdma: Add support to re-register a memory region")
> Signed-off-by: Jacob Moroni <jmoroni@google.com>

Sashiko has reviewed this patch and found no issues.

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