Re: [PATCH v3 for-next] RDMA/rxe: Fix error paths in MR alloc routines

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/08/2022 06:41, Bob Pearson wrote:
> 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. Setting mr->ibmr.pd is removed since this
> should always take place in rdma-core.
>
> Reported-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
> Link: https://lore.kernel.org/linux-rdma/11dafa5f-c52d-16c1-fe37-2cd45ab20474@xxxxxxxxxxx/

I haven't reviewed the details, but it still cause the kernel crash after force injecting an error.

diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
index 56940cf70fe0..bf72b9834575 100644
--- a/drivers/infiniband/sw/rxe/rxe_mr.c
+++ b/drivers/infiniband/sw/rxe/rxe_mr.c
@@ -127,7 +127,7 @@ int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length, u64 iova,

         rxe_mr_init(access, mr);

-       err = rxe_mr_alloc(mr, num_buf);
+       err = -ENOMEM; // rxe_mr_alloc(mr, num_buf);
         if (err) {
                 ib_umem_release(umem);
                 return err;




> Fixes: 3902b429ca14 ("Implement invalidate MW operations")
> Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
> ---
> 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.
> ---
>   drivers/infiniband/sw/rxe/rxe_loc.h   |  6 +-
>   drivers/infiniband/sw/rxe/rxe_mr.c    | 91 +++++++++++----------------
>   drivers/infiniband/sw/rxe/rxe_verbs.c | 53 ++++++----------
>   3 files changed, 57 insertions(+), 93 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..56940cf70fe0 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -67,20 +67,24 @@ static void rxe_mr_init(int access, struct rxe_mr *mr)
>   
>   static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf)
>   {
> -	int i;
> -	int num_map;
>   	struct rxe_map **map = mr->map;
> +	int num_map;
> +	int i;
>   
>   	num_map = (num_buf + RXE_BUF_PER_MAP - 1) / RXE_BUF_PER_MAP;
>   
>   	mr->map = kmalloc_array(num_map, sizeof(*map), GFP_KERNEL);
>   	if (!mr->map)
> -		goto err1;
> +		return -ENOMEM;
>   
>   	for (i = 0; i < num_map; i++) {
>   		mr->map[i] = kmalloc(sizeof(**map), GFP_KERNEL);
> -		if (!mr->map[i])
> -			goto err2;
> +		if (!mr->map[i]) {
> +			for (i--; i >= 0; i--)
> +				kfree(mr->map[i]);
> +			kfree(mr->map);
> +			return -ENOMEM;
> +		}
>   	}
>   
>   	BUILD_BUG_ON(!is_power_of_2(RXE_BUF_PER_MAP));
> @@ -93,45 +97,31 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf)
>   	mr->max_buf = num_map * RXE_BUF_PER_MAP;
>   
>   	return 0;
> -
> -err2:
> -	for (i--; i >= 0; i--)
> -		kfree(mr->map[i]);
> -
> -	kfree(mr->map);
> -err1:
> -	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;
> -	struct rxe_phys_buf	*buf = NULL;
> -	struct ib_umem		*umem;
> -	struct sg_page_iter	sg_iter;
> -	int			num_buf;
> -	void			*vaddr;
> +	struct rxe_phys_buf *buf = NULL;
> +	struct sg_page_iter sg_iter;
> +	struct rxe_map **map;
> +	struct ib_umem *umem;
> +	int num_buf;
> +	void *vaddr;
>   	int err;
> -	int i;
>   
> -	umem = ib_umem_get(pd->ibpd.device, start, length, access);
> -	if (IS_ERR(umem)) {
> -		pr_warn("%s: Unable to pin memory region err = %d\n",
> -			__func__, (int)PTR_ERR(umem));
> -		err = PTR_ERR(umem);
> -		goto err_out;
> -	}
> +	umem = ib_umem_get(&rxe->ib_dev, start, length, access);
> +	if (IS_ERR(umem))
> +		return PTR_ERR(umem);
>   
>   	num_buf = ib_umem_num_pages(umem);
>   
> @@ -139,9 +129,8 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   
>   	err = rxe_mr_alloc(mr, num_buf);
>   	if (err) {
> -		pr_warn("%s: Unable to allocate memory for map\n",
> -				__func__);
> -		goto err_release_umem;
> +		ib_umem_release(umem);
> +		return err;
>   	}
>   
>   	mr->page_shift = PAGE_SHIFT;
> @@ -152,7 +141,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   	if (length > 0) {
>   		buf = map[0]->buf;
>   
> -		for_each_sgtable_page (&umem->sgt_append.sgt, &sg_iter, 0) {
> +		for_each_sgtable_page(&umem->sgt_append.sgt, &sg_iter, 0) {
>   			if (num_buf >= RXE_BUF_PER_MAP) {
>   				map++;
>   				buf = map[0]->buf;
> @@ -161,21 +150,22 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   
>   			vaddr = page_address(sg_page_iter_page(&sg_iter));
>   			if (!vaddr) {
> -				pr_warn("%s: Unable to get virtual address\n",
> -						__func__);
> -				err = -ENOMEM;
> -				goto err_cleanup_map;
> +				int i;
> +
> +				for (i = 0; i < mr->num_map; i++)
> +					kfree(mr->map[i]);
> +				kfree(mr->map);
> +				ib_umem_release(umem);
> +				return -ENOMEM;
>   			}
>   
>   			buf->addr = (uintptr_t)vaddr;
>   			buf->size = PAGE_SIZE;
>   			num_buf++;
>   			buf++;
> -
>   		}
>   	}
>   
> -	mr->ibmr.pd = &pd->ibpd;
>   	mr->umem = umem;
>   	mr->access = access;
>   	mr->length = length;
> @@ -186,18 +176,9 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   	mr->type = IB_MR_TYPE_USER;
>   
>   	return 0;
> -
> -err_cleanup_map:
> -	for (i = 0; i < mr->num_map; i++)
> -		kfree(mr->map[i]);
> -	kfree(mr->map);
> -err_release_umem:
> -	ib_umem_release(umem);
> -err_out:

Could you keep this goto label, i have WIP code which needs such error path.

Thanks
Zhijian
> base-commit: 6b822d408b58c3c4f26dae93245c6b7d8b39e0f9




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux