[PATCH v5 for-next 0/2] RDMA/rxe: Fix error paths in MR alloc routines

[PATCH v5 for-next 0/2] RDMA/rxe: Fix error paths in MR alloc routines

[Bob Pearson]

Currently the rxe driver has incorrect code in error paths for
allocating MR objects. The PD and umem are always freed in
rxe_mr_cleanup() but in some error paths they are already
freed or never set. This patch makes sure that the PD is always
set and checks to see if umem is set before freeing it in
rxe_mr_cleanup(). umem and maps are freed if an error occurs
in an allocate mr call.

Reported-by: Li Zhijian <lizhijian@fujitsu.com>
Link: https://lore.kernel.org/linux-rdma/11dafa5f-c52d-16c1-fe37-2cd45ab20474@fujitsu.com/
Fixes: 3902b429ca14 ("Implement invalidate MW operations")
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
v5:
  Dropped cleanup code from patch per Li Zhijian.
  Split up into two small patches.
v4:
  Added set mr->ibmr.pd back to avoid an -ENOMEM error causing
  rxe_put(mr_pd(mr)); to seg fault in rxe_mr_cleanup() since pd
  is not getting set in the error path.
v3:
  Rebased to apply to current for-next after
  	Revert "RDMA/rxe: Create duplicate mapping tables for FMRs"
v2:
  Moved setting mr->umem until after checks to avoid sending
  an ERR_PTR to ib_umem_release().
  Cleaned up umem and map sets if errors occur in alloc mr calls.
  Rebased to current for-next.

Bob Pearson (2):
  RDMA/rxe: Set pd early in mr alloc routines
  RDMA/rxe: Test mr->umem before releaseing umem

 drivers/infiniband/sw/rxe/rxe_loc.h   |  6 +++---
 drivers/infiniband/sw/rxe/rxe_mr.c    | 15 +++++++--------
 drivers/infiniband/sw/rxe/rxe_verbs.c | 10 +++++++---
 3 files changed, 17 insertions(+), 14 deletions(-)


base-commit: 6b822d408b58c3c4f26dae93245c6b7d8b39e0f9
-- 
2.34.1

[PATCH v5 1/2] RDMA/rxe: Set pd early in mr alloc routines

[Bob Pearson]

Move setting of pd in mr objects ahead of any possible errors
so that it will always be set in rxe_mr_complete() to avoid
seg faults when rxe_put(mr_pd(mr)) is called.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_loc.h   |  6 +++---
 drivers/infiniband/sw/rxe/rxe_mr.c    | 11 ++++-------
 drivers/infiniband/sw/rxe/rxe_verbs.c | 10 +++++++---
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index 22f6cc31d1d6..c2a5c8814a48 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -64,10 +64,10 @@ int rxe_mmap(struct ib_ucontext *context, struct vm_area_struct *vma);
 
 /* rxe_mr.c */
 u8 rxe_get_next_key(u32 last_key);
-void rxe_mr_init_dma(struct rxe_pd *pd, int access, struct rxe_mr *mr);
-int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
+void rxe_mr_init_dma(int access, struct rxe_mr *mr);
+int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
 		     int access, struct rxe_mr *mr);
-int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr);
+int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr);
 int rxe_mr_copy(struct rxe_mr *mr, u64 iova, void *addr, int length,
 		enum rxe_mr_copy_dir dir);
 int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 850b80f5ad8b..af34f198e645 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -103,17 +103,16 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf)
 	return -ENOMEM;
 }
 
-void rxe_mr_init_dma(struct rxe_pd *pd, int access, struct rxe_mr *mr)
+void rxe_mr_init_dma(int access, struct rxe_mr *mr)
 {
 	rxe_mr_init(access, mr);
 
-	mr->ibmr.pd = &pd->ibpd;
 	mr->access = access;
 	mr->state = RXE_MR_STATE_VALID;
 	mr->type = IB_MR_TYPE_DMA;
 }
 
-int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
+int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,
 		     int access, struct rxe_mr *mr)
 {
 	struct rxe_map		**map;
@@ -125,7 +124,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
 	int err;
 	int i;
 
-	umem = ib_umem_get(pd->ibpd.device, start, length, access);
+	umem = ib_umem_get(&rxe->ib_dev, start, length, access);
 	if (IS_ERR(umem)) {
 		pr_warn("%s: Unable to pin memory region err = %d\n",
 			__func__, (int)PTR_ERR(umem));
@@ -175,7 +174,6 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
 		}
 	}
 
-	mr->ibmr.pd = &pd->ibpd;
 	mr->umem = umem;
 	mr->access = access;
 	mr->length = length;
@@ -197,7 +195,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
 	return err;
 }
 
-int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr)
+int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
 {
 	int err;
 
@@ -208,7 +206,6 @@ int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr)
 	if (err)
 		goto err1;
 
-	mr->ibmr.pd = &pd->ibpd;
 	mr->max_buf = max_pages;
 	mr->state = RXE_MR_STATE_FREE;
 	mr->type = IB_MR_TYPE_MEM_REG;
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index e264cf69bf55..6c13be14d723 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -903,7 +903,9 @@ static struct ib_mr *rxe_get_dma_mr(struct ib_pd *ibpd, int access)
 		return ERR_PTR(-ENOMEM);
 
 	rxe_get(pd);
-	rxe_mr_init_dma(pd, access, mr);
+	mr->ibmr.pd = ibpd;
+
+	rxe_mr_init_dma(access, mr);
 	rxe_finalize(mr);
 
 	return &mr->ibmr;
@@ -928,8 +930,9 @@ static struct ib_mr *rxe_reg_user_mr(struct ib_pd *ibpd,
 
 
 	rxe_get(pd);
+	mr->ibmr.pd = ibpd;
 
-	err = rxe_mr_init_user(pd, start, length, iova, access, mr);
+	err = rxe_mr_init_user(rxe, start, length, iova, access, mr);
 	if (err)
 		goto err3;
 
@@ -962,8 +965,9 @@ static struct ib_mr *rxe_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type,
 	}
 
 	rxe_get(pd);
+	mr->ibmr.pd = ibpd;
 
-	err = rxe_mr_init_fast(pd, max_num_sg, mr);
+	err = rxe_mr_init_fast(max_num_sg, mr);
 	if (err)
 		goto err2;
 
-- 
2.34.1

Re: [PATCH v5 1/2] RDMA/rxe: Set pd early in mr alloc routines

[Bob Pearson]

On 8/5/22 13:31, Bob Pearson wrote:
> Move setting of pd in mr objects ahead of any possible errors
> so that it will always be set in rxe_mr_complete() to avoid
> seg faults when rxe_put(mr_pd(mr)) is called.
> 
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>

Ignore I missed the for-next

Re: [PATCH v5 1/2] RDMA/rxe: Set pd early in mr alloc routines

[Jason Gunthorpe]

On Fri, Aug 05, 2022 at 01:31:54PM -0500, Bob Pearson wrote:
> Move setting of pd in mr objects ahead of any possible errors
> so that it will always be set in rxe_mr_complete() to avoid

This means to say rxe_mr_cleanup()

> seg faults when rxe_put(mr_pd(mr)) is called.
> 
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_loc.h   |  6 +++---
>  drivers/infiniband/sw/rxe/rxe_mr.c    | 11 ++++-------
>  drivers/infiniband/sw/rxe/rxe_verbs.c | 10 +++++++---
>  3 files changed, 14 insertions(+), 13 deletions(-)

Applied to for-next

I dropped the next patch since it is a NOP due to the protection in
ib_umem_release()

Thanks,
Jason

[PATCH v5 for-next 2/2] RDMA/rxe: Test mr->umem before releasing umem

[Bob Pearson]

In rxe_mr_cleanup() test mr->umem before calling ib_umem_release()
since in some error paths it may not be set.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_mr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index af34f198e645..f0726e8ee855 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -627,7 +627,9 @@ void rxe_mr_cleanup(struct rxe_pool_elem *elem)
 	int i;
 
 	rxe_put(mr_pd(mr));
-	ib_umem_release(mr->umem);
+
+	if (mr->umem)
+		ib_umem_release(mr->umem);
 
 	if (mr->map) {
 		for (i = 0; i < mr->num_map; i++)
-- 
2.34.1

RE: [PATCH v5 for-next 2/2] RDMA/rxe: Test mr->umem before releasing umem

[lizhijian]

> -----Original Message-----
> From: Bob Pearson <rpearsonhpe@gmail.com>
> Sent: Saturday, August 6, 2022 2:32 AM
> To: jgg@nvidia.com; zyjzyj2000@gmail.com; Li, Zhijian/李 智坚
> <lizhijian@fujitsu.com>; jhack@hpe.com; linux-rdma@vger.kernel.org
> Cc: Bob Pearson <rpearsonhpe@gmail.com>
> Subject: [PATCH v5 for-next 2/2] RDMA/rxe: Test mr->umem before releasing
> umem
> 
> In rxe_mr_cleanup() test mr->umem before calling ib_umem_release() since in
> some error paths it may not be set.
> 
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_mr.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c
> b/drivers/infiniband/sw/rxe/rxe_mr.c
> index af34f198e645..f0726e8ee855 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -627,7 +627,9 @@ void rxe_mr_cleanup(struct rxe_pool_elem *elem)
>  	int i;
> 
>  	rxe_put(mr_pd(mr));
> -	ib_umem_release(mr->umem);
> +
> +	if (mr->umem)
> +		ib_umem_release(mr->umem);

It's safe to pass a NULL to ib_umem_release() where it has a such check.


Thanks
Zhijian

> 
>  	if (mr->map) {
>  		for (i = 0; i < mr->num_map; i++)
> --
> 2.34.1

Re: [PATCH v5 for-next 2/2] RDMA/rxe: Test mr->umem before releasing umem

[Bob Pearson]

On 8/22/22 00:50, lizhijian@fujitsu.com wrote:
> 
> 
>> -----Original Message-----
>> From: Bob Pearson <rpearsonhpe@gmail.com>
>> Sent: Saturday, August 6, 2022 2:32 AM
>> To: jgg@nvidia.com; zyjzyj2000@gmail.com; Li, Zhijian/李 智坚
>> <lizhijian@fujitsu.com>; jhack@hpe.com; linux-rdma@vger.kernel.org
>> Cc: Bob Pearson <rpearsonhpe@gmail.com>
>> Subject: [PATCH v5 for-next 2/2] RDMA/rxe: Test mr->umem before releasing
>> umem
>>
>> In rxe_mr_cleanup() test mr->umem before calling ib_umem_release() since in
>> some error paths it may not be set.
>>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>> ---
>>  drivers/infiniband/sw/rxe/rxe_mr.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c
>> b/drivers/infiniband/sw/rxe/rxe_mr.c
>> index af34f198e645..f0726e8ee855 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
>> @@ -627,7 +627,9 @@ void rxe_mr_cleanup(struct rxe_pool_elem *elem)
>>  	int i;
>>
>>  	rxe_put(mr_pd(mr));
>> -	ib_umem_release(mr->umem);
>> +
>> +	if (mr->umem)
>> +		ib_umem_release(mr->umem);
> 
> It's safe to pass a NULL to ib_umem_release() where it has a such check.
> 
> 
> Thanks
> Zhijian
> 
>>
>>  	if (mr->map) {
>>  		for (i = 0; i < mr->num_map; i++)
>> --
>> 2.34.1
> 
OK we can just drop this patch.

Bob