On 16/07/2022 02:24, 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(). > > Reported-by: Li Zhijian <lizhijian@xxxxxxxxxxx> > Link: https://lore.kernel.org/linux-rdma/11dafa5f-c52d-16c1-fe37-2cd45ab20474@xxxxxxxxxxx/ > Fixes: 3902b429ca14 ("Implement invalidate MW operations") > Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx> > --- > drivers/infiniband/sw/rxe/rxe_loc.h | 6 ++-- > drivers/infiniband/sw/rxe/rxe_mr.c | 36 +++++++------------- > drivers/infiniband/sw/rxe/rxe_verbs.c | 47 +++++++++++---------------- > 3 files changed, 34 insertions(+), 55 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h > index 0e022ae1b8a5..8969918275f9 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 9a5c2af6a56f..674af4c36c49 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -151,17 +151,16 @@ static int rxe_mr_alloc(struct rxe_mr *mr, int num_buf, int both) > 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_set *set; > @@ -173,12 +172,11 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > void *vaddr; > int err; > > - umem = ib_umem_get(pd->ibpd.device, start, length, access); > + mr->umem = umem = ib_umem_get(&rxe->ib_dev, start, length, access); > if (IS_ERR(umem)) { In this case, mr->umem is not NULL as well, which will confuse the releasing patch below + if (mr->umem) + ib_umem_release(mr->umem); IMHO, by convention, we should follow the rule: destroy/free the object allocated in the same routine when the routine got something wrong. @Jason, Yanjun, what's your opinion. Thanks Zhijian > pr_warn("%s: Unable to pin memory region err = %d\n", > __func__, (int)PTR_ERR(umem)); > - err = PTR_ERR(umem); > - goto err_out; > + return PTR_ERR(umem); > } > > num_buf = ib_umem_num_pages(umem); > @@ -189,7 +187,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > if (err) { > pr_warn("%s: Unable to allocate memory for map\n", > __func__); > - goto err_release_umem; > + return err; > } > > set = mr->cur_map_set; > @@ -213,8 +211,7 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > if (!vaddr) { > pr_warn("%s: Unable to get virtual address\n", > __func__); > - err = -ENOMEM; > - goto err_release_umem; > + return -ENOMEM; > } > > buf->addr = (uintptr_t)vaddr; > @@ -224,8 +221,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->state = RXE_MR_STATE_VALID; > mr->type = IB_MR_TYPE_USER; > @@ -236,14 +231,9 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > set->offset = ib_umem_offset(umem); > > return 0; > - > -err_release_umem: > - ib_umem_release(umem); > -err_out: > - 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; > > @@ -252,17 +242,13 @@ int rxe_mr_init_fast(struct rxe_pd *pd, int max_pages, struct rxe_mr *mr) > > err = rxe_mr_alloc(mr, max_pages, 1); > if (err) > - goto err1; > + return err; > > - mr->ibmr.pd = &pd->ibpd; > mr->max_buf = max_pages; > mr->state = RXE_MR_STATE_FREE; > mr->type = IB_MR_TYPE_MEM_REG; > > return 0; > - > -err1: > - return err; > } > > static void lookup_iova(struct rxe_mr *mr, u64 iova, int *m_out, int *n_out, > @@ -695,10 +681,12 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) > void rxe_mr_cleanup(struct rxe_pool_elem *elem) > { > struct rxe_mr *mr = container_of(elem, typeof(*mr), elem); > + struct rxe_pd *pd = mr_pd(mr); > > - rxe_put(mr_pd(mr)); > + rxe_put(pd); > > - ib_umem_release(mr->umem); > + if (mr->umem) > + ib_umem_release(mr->umem); > > if (mr->cur_map_set) > rxe_mr_free_map_set(mr->num_map, mr->cur_map_set); > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c > index 151c6280abd5..173172a83c74 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; > @@ -921,27 +923,21 @@ static struct ib_mr *rxe_reg_user_mr(struct ib_pd *ibpd, > struct rxe_mr *mr; > > mr = rxe_alloc(&rxe->mr_pool); > - if (!mr) { > - err = -ENOMEM; > - goto err2; > - } > - > + if (!mr) > + return ERR_PTR(-ENOMEM); > > rxe_get(pd); > + mr->ibmr.pd = ibpd; > > - err = rxe_mr_init_user(pd, start, length, iova, access, mr); > - if (err) > - goto err3; > + err = rxe_mr_init_user(rxe, start, length, iova, access, mr); > + if (err) { > + rxe_cleanup(mr); > + return ERR_PTR(err); > + } > > rxe_finalize(mr); > > return &mr->ibmr; > - > -err3: > - rxe_put(pd); > - rxe_cleanup(mr); > -err2: > - return ERR_PTR(err); > } > > static struct ib_mr *rxe_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type, > @@ -956,26 +952,21 @@ static struct ib_mr *rxe_alloc_mr(struct ib_pd *ibpd, enum ib_mr_type mr_type, > return ERR_PTR(-EINVAL); > > mr = rxe_alloc(&rxe->mr_pool); > - if (!mr) { > - err = -ENOMEM; > - goto err1; > - } > + if (!mr) > + return ERR_PTR(-ENOMEM); > > rxe_get(pd); > + mr->ibmr.pd = ibpd; > > - err = rxe_mr_init_fast(pd, max_num_sg, mr); > - if (err) > - goto err2; > + err = rxe_mr_init_fast(max_num_sg, mr); > + if (err) { > + rxe_cleanup(mr); > + return ERR_PTR(err); > + } > > rxe_finalize(mr); > > return &mr->ibmr; > - > -err2: > - rxe_put(pd); > - rxe_cleanup(mr); > -err1: > - return ERR_PTR(err); > } > > /* build next_map_set from scatterlist > > base-commit: 2635d2a8d4664b665bc12e15eee88e9b1b40ae7b
