Messages in this thread | | | Date | Mon, 18 Jul 2022 15:30:43 +0300 | From | Leon Romanovsky <> | Subject | Re: [PATCH] RDMA/hfi1: fix potential memory leak in setup_base_ctxt() |
| |
On Mon, Jul 18, 2022 at 08:11:59AM -0400, Dennis Dalessandro wrote: > On 7/18/22 6:39 AM, Leon Romanovsky wrote: > > On Mon, Jul 11, 2022 at 07:52:25AM -0400, Dennis Dalessandro wrote: > >> On 7/11/22 3:07 AM, Jianglei Nie wrote: > >>> setup_base_ctxt() allocates a memory chunk for uctxt->groups with > >>> hfi1_alloc_ctxt_rcv_groups(). When init_user_ctxt() fails, uctxt->groups > >>> is not released, which will lead to a memory leak. > >>> > >>> We should release the uctxt->groups with hfi1_free_ctxt_rcv_groups() > >>> when init_user_ctxt() fails. > >>> > >>> Signed-off-by: Jianglei Nie <niejianglei2021@163.com> > >>> --- > >>> drivers/infiniband/hw/hfi1/file_ops.c | 4 +++- > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c > >>> index 2e4cf2b11653..629beff053ad 100644 > >>> --- a/drivers/infiniband/hw/hfi1/file_ops.c > >>> +++ b/drivers/infiniband/hw/hfi1/file_ops.c > >>> @@ -1179,8 +1179,10 @@ static int setup_base_ctxt(struct hfi1_filedata *fd, > >>> goto done; > >>> > >>> ret = init_user_ctxt(fd, uctxt); > >>> - if (ret) > >>> + if (ret) { > >>> + hfi1_free_ctxt_rcv_groups(uctxt); > >>> goto done; > >>> + } > >>> > >>> user_init(uctxt); > >>> > >> > >> Doesn't seem like this patch is correct. The free is done when the file is > >> closed, along with other clean up stuff. See hfi1_file_close(). > > > > Can setup_base_ctxt() be called twice for same uctxt? > > You are allocating rcd->groups and not releasing. > > The first thing assign_ctxt() does is a check of the fd->uctxt and it bails with > -EINVAL. So effectively only once.
I'm slightly confused. How will you release rcd->groups?
assign_ctxt() -> setup_base_ctxt() -> hfi1_alloc_ctxt_rcv_groups() ,,, rcd->groups = kzalloc... ... -> init_user_ctxt() <-- fails and leaves fd->uctx == NULL
... hfi1_file_close() struct hfi1_ctxtdata *uctxt = fdata->uctxt; ... if (!uctxt) <-- This is our case goto done; ...
done: if (refcount_dec_and_test(&dd->user_refcount)) complete(&dd->user_comp);
cleanup_srcu_struct(&fdata->pq_srcu); kfree(fdata); return 0;
> > -Denny > >
| |