| Message ID | 20200821081013.4762-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/uverbs: Fix memleak in ib_uverbs_add_one | expand |
> On 21 Aug 2020, at 10:10, Dinghao Liu <dinghao.liu@zju.edu.cn> wrote: > > When ida_alloc_max() fails, uverbs_dev should be freed > just like when init_srcu_struct() fails. It's the same > for the error paths after this call. > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > --- > drivers/infiniband/core/uverbs_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > index 37794d88b1f3..c6b4e3e2aff6 100644 > --- a/drivers/infiniband/core/uverbs_main.c > +++ b/drivers/infiniband/core/uverbs_main.c > @@ -1170,6 +1170,7 @@ static int ib_uverbs_add_one(struct ib_device *device) > ib_uverbs_comp_dev(uverbs_dev); > wait_for_completion(&uverbs_dev->comp); > put_device(&uverbs_dev->dev); > + kfree(uverbs_dev); Isn't this taken care of by the *release* function pointer, which happens to be ib_uverbs_release_dev() ? Thxs, Håkon > return ret; > } > > -- > 2.17.1 >
On Fri, Aug 21, 2020 at 11:47:32AM +0200, Håkon Bugge wrote: > > > > On 21 Aug 2020, at 10:10, Dinghao Liu <dinghao.liu@zju.edu.cn> wrote: > > > > When ida_alloc_max() fails, uverbs_dev should be freed > > just like when init_srcu_struct() fails. It's the same > > for the error paths after this call. > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > drivers/infiniband/core/uverbs_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > > index 37794d88b1f3..c6b4e3e2aff6 100644 > > +++ b/drivers/infiniband/core/uverbs_main.c > > @@ -1170,6 +1170,7 @@ static int ib_uverbs_add_one(struct ib_device *device) > > ib_uverbs_comp_dev(uverbs_dev); > > wait_for_completion(&uverbs_dev->comp); > > put_device(&uverbs_dev->dev); > > + kfree(uverbs_dev); > > Isn't this taken care of by the *release* function pointer, which > happens to be ib_uverbs_release_dev() ? Yep Jason
> > > On 21 Aug 2020, at 10:10, Dinghao Liu <dinghao.liu@zju.edu.cn> wrote: > > > > When ida_alloc_max() fails, uverbs_dev should be freed > > just like when init_srcu_struct() fails. It's the same > > for the error paths after this call. > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > --- > > drivers/infiniband/core/uverbs_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > > index 37794d88b1f3..c6b4e3e2aff6 100644 > > --- a/drivers/infiniband/core/uverbs_main.c > > +++ b/drivers/infiniband/core/uverbs_main.c > > @@ -1170,6 +1170,7 @@ static int ib_uverbs_add_one(struct ib_device *device) > > ib_uverbs_comp_dev(uverbs_dev); > > wait_for_completion(&uverbs_dev->comp); > > put_device(&uverbs_dev->dev); > > + kfree(uverbs_dev); > > Isn't this taken care of by the *release* function pointer, which happens to be ib_uverbs_release_dev() ? > You are right, thank you for pointing out that! Regards, Dinghao
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 37794d88b1f3..c6b4e3e2aff6 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -1170,6 +1170,7 @@ static int ib_uverbs_add_one(struct ib_device *device) ib_uverbs_comp_dev(uverbs_dev); wait_for_completion(&uverbs_dev->comp); put_device(&uverbs_dev->dev); + kfree(uverbs_dev); return ret; }
When ida_alloc_max() fails, uverbs_dev should be freed just like when init_srcu_struct() fails. It's the same for the error paths after this call. Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- drivers/infiniband/core/uverbs_main.c | 1 + 1 file changed, 1 insertion(+)