| Message ID | 20220711070718.2318320-1-niejianglei2021@163.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | RDMA/hfi1: fix potential memory leak in setup_base_ctxt() | expand |
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(). -Denny
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. Thanks > > -Denny
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. -Denny
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 > >
On 7/18/22 8:30 AM, Leon Romanovsky wrote: > 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; > Looks like this may have been broken with: e87473bc1b6c ("IB/hfi1: Only set fd pointer when base context is completely initialized") The question is does it make more sense to just move the fd->uctxt assignment up, or call the free directly. I think that might be opening a bigger can of worms though, as this was part of a larger patch set. Maybe it is best after all to go with this patch. Let's add the above as a fixes line and tack on: Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> It's been like this since 4.14, so no rush to get it in for the ultra late RC. I'll get it tested as part of the next cycle. -Denny
On Mon, Jul 18, 2022 at 09:56:48AM -0400, Dennis Dalessandro wrote: > On 7/18/22 8:30 AM, Leon Romanovsky wrote: > > 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; > > > > Looks like this may have been broken with: > > e87473bc1b6c ("IB/hfi1: Only set fd pointer when base context is completely > initialized") > > The question is does it make more sense to just move the fd->uctxt assignment > up, or call the free directly. I think that might be opening a bigger can of > worms though, as this was part of a larger patch set. Maybe it is best after all > to go with this patch. > > Let's add the above as a fixes line and tack on: > > Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> > > It's been like this since 4.14, so no rush to get it in for the ultra late RC. > I'll get it tested as part of the next cycle. > Thanks, applied.
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);
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(-)