| Message ID | 20220120143237.63374-1-jinpu.wang@ionos.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rtrs-clt: Fix possible double free in error case | expand |
On Thu, Jan 20, 2022 at 3:32 PM Jack Wang <jinpu.wang@ionos.com> wrote: > > Callback function rtrs_clt_dev_release() for put_device() > calls kfree(clt) to free memory. We shouldn't call kfree(clt) again, > and we can't use the clt after kfree too. > > Fixes: 6a98d71daea1 ("RDMA/rtrs: client: main functionality") > Reported-by: Miaoqian Lin <linmq006@gmail.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/infiniband/ulp/rtrs/rtrs-clt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > index b159471a8959..fbce9cb87d08 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > @@ -2680,6 +2680,7 @@ static void rtrs_clt_dev_release(struct device *dev) > struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess, > dev); > > + free_percpu(clt->pcpu_path); > kfree(clt); > } > > @@ -2760,8 +2761,6 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num, > err_dev: > device_unregister(&clt->dev); > err: > - free_percpu(clt->pcpu_path); > - kfree(clt); If dev_set_name fails, it would end up not calling the release function since device_register has not been called yet, hence pcpu_path, clt wont be freed. Sending another patch in sometime > return ERR_PTR(err); > } > > -- > 2.25.1 >
On Thu, Jan 20, 2022 at 03:32:37PM +0100, Jack Wang wrote: > Callback function rtrs_clt_dev_release() for put_device() > calls kfree(clt) to free memory. We shouldn't call kfree(clt) again, > and we can't use the clt after kfree too. > > Fixes: 6a98d71daea1 ("RDMA/rtrs: client: main functionality") > Reported-by: Miaoqian Lin <linmq006@gmail.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > drivers/infiniband/ulp/rtrs/rtrs-clt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > index b159471a8959..fbce9cb87d08 100644 > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > @@ -2680,6 +2680,7 @@ static void rtrs_clt_dev_release(struct device *dev) > struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess, > dev); > > + free_percpu(clt->pcpu_path); > kfree(clt); > } This need to delete the call in free_clt() too. Also, calling dev_set_name before device_initialize is a bad idea. Do it like this and fix all the bugs please: diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c index b696aa4abae46d..4d1895ab99c4da 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c @@ -2685,6 +2685,9 @@ static void rtrs_clt_dev_release(struct device *dev) struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess, dev); + free_percpu(clt->pcpu_path); + mutex_destroy(&clt->paths_ev_mutex); + mutex_destroy(&clt->paths_mutex); kfree(clt); } @@ -2707,13 +2710,8 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num, clt = kzalloc(sizeof(*clt), GFP_KERNEL); if (!clt) return ERR_PTR(-ENOMEM); - - clt->pcpu_path = alloc_percpu(typeof(*clt->pcpu_path)); - if (!clt->pcpu_path) { - kfree(clt); - return ERR_PTR(-ENOMEM); - } - + clt->dev.class = rtrs_clt_dev_class; + clt->dev.release = rtrs_clt_dev_release; uuid_gen(&clt->paths_uuid); INIT_LIST_HEAD_RCU(&clt->paths_list); clt->paths_num = paths_num; @@ -2730,52 +2728,52 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num, init_waitqueue_head(&clt->permits_wait); mutex_init(&clt->paths_ev_mutex); mutex_init(&clt->paths_mutex); + device_initialize(&clt->dev); + + clt->pcpu_path = alloc_percpu(typeof(*clt->pcpu_path)); + if (!clt->pcpu_path) { + err = -ENOMEM; + goto err_put; + } - clt->dev.class = rtrs_clt_dev_class; - clt->dev.release = rtrs_clt_dev_release; err = dev_set_name(&clt->dev, "%s", sessname); if (err) - goto err; + goto err_put; + /* * Suppress user space notification until * sysfs files are created */ dev_set_uevent_suppress(&clt->dev, true); - err = device_register(&clt->dev); - if (err) { - put_device(&clt->dev); - goto err; - } + err = device_add(&clt->dev); + if (err) + goto err_put; clt->kobj_paths = kobject_create_and_add("paths", &clt->dev.kobj); if (!clt->kobj_paths) { err = -ENOMEM; - goto err_dev; + goto err_del; } err = rtrs_clt_create_sysfs_root_files(clt); if (err) { kobject_del(clt->kobj_paths); kobject_put(clt->kobj_paths); - goto err_dev; + goto err_del; } dev_set_uevent_suppress(&clt->dev, false); kobject_uevent(&clt->dev.kobj, KOBJ_ADD); return clt; -err_dev: - device_unregister(&clt->dev); -err: - free_percpu(clt->pcpu_path); - kfree(clt); +err_del: + device_del(&clt->dev); +err_put: + put_device(&clt->dev); return ERR_PTR(err); } static void free_clt(struct rtrs_clt_sess *clt) { free_permits(clt); - free_percpu(clt->pcpu_path); - mutex_destroy(&clt->paths_ev_mutex); - mutex_destroy(&clt->paths_mutex); /* release callback will free clt in last put */ device_unregister(&clt->dev); }
On Fri, Jan 28, 2022 at 5:59 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Thu, Jan 20, 2022 at 03:32:37PM +0100, Jack Wang wrote: > > Callback function rtrs_clt_dev_release() for put_device() > > calls kfree(clt) to free memory. We shouldn't call kfree(clt) again, > > and we can't use the clt after kfree too. > > > > Fixes: 6a98d71daea1 ("RDMA/rtrs: client: main functionality") > > Reported-by: Miaoqian Lin <linmq006@gmail.com> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > drivers/infiniband/ulp/rtrs/rtrs-clt.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > > index b159471a8959..fbce9cb87d08 100644 > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > > @@ -2680,6 +2680,7 @@ static void rtrs_clt_dev_release(struct device *dev) > > struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess, > > dev); > > > > + free_percpu(clt->pcpu_path); > > kfree(clt); > > } > > This need to delete the call in free_clt() too. > > Also, calling dev_set_name before device_initialize is a bad idea. > > Do it like this and fix all the bugs please: > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > index b696aa4abae46d..4d1895ab99c4da 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c > @@ -2685,6 +2685,9 @@ static void rtrs_clt_dev_release(struct device *dev) > struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess, > dev); > > + free_percpu(clt->pcpu_path); > + mutex_destroy(&clt->paths_ev_mutex); > + mutex_destroy(&clt->paths_mutex); > kfree(clt); > } > > @@ -2707,13 +2710,8 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num, > clt = kzalloc(sizeof(*clt), GFP_KERNEL); > if (!clt) > return ERR_PTR(-ENOMEM); > - > - clt->pcpu_path = alloc_percpu(typeof(*clt->pcpu_path)); > - if (!clt->pcpu_path) { > - kfree(clt); > - return ERR_PTR(-ENOMEM); > - } > - > + clt->dev.class = rtrs_clt_dev_class; > + clt->dev.release = rtrs_clt_dev_release; > uuid_gen(&clt->paths_uuid); > INIT_LIST_HEAD_RCU(&clt->paths_list); > clt->paths_num = paths_num; > @@ -2730,52 +2728,52 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num, > init_waitqueue_head(&clt->permits_wait); > mutex_init(&clt->paths_ev_mutex); > mutex_init(&clt->paths_mutex); > + device_initialize(&clt->dev); > + > + clt->pcpu_path = alloc_percpu(typeof(*clt->pcpu_path)); > + if (!clt->pcpu_path) { > + err = -ENOMEM; > + goto err_put; This path would lead to a call to "free_percpu(clt->pcpu_path);", even after alloc_percpu failed. Everything else looks good to me. I will send a revised patch after some internal testing in sometime. Thanks for the review and comments. > + } > > - clt->dev.class = rtrs_clt_dev_class; > - clt->dev.release = rtrs_clt_dev_release; > err = dev_set_name(&clt->dev, "%s", sessname); > if (err) > - goto err; > + goto err_put; > + > /* > * Suppress user space notification until > * sysfs files are created > */ > dev_set_uevent_suppress(&clt->dev, true); > - err = device_register(&clt->dev); > - if (err) { > - put_device(&clt->dev); > - goto err; > - } > + err = device_add(&clt->dev); > + if (err) > + goto err_put; > > clt->kobj_paths = kobject_create_and_add("paths", &clt->dev.kobj); > if (!clt->kobj_paths) { > err = -ENOMEM; > - goto err_dev; > + goto err_del; > } > err = rtrs_clt_create_sysfs_root_files(clt); > if (err) { > kobject_del(clt->kobj_paths); > kobject_put(clt->kobj_paths); > - goto err_dev; > + goto err_del; > } > dev_set_uevent_suppress(&clt->dev, false); > kobject_uevent(&clt->dev.kobj, KOBJ_ADD); > > return clt; > -err_dev: > - device_unregister(&clt->dev); > -err: > - free_percpu(clt->pcpu_path); > - kfree(clt); > +err_del: > + device_del(&clt->dev); > +err_put: > + put_device(&clt->dev); > return ERR_PTR(err); > } > > static void free_clt(struct rtrs_clt_sess *clt) > { > free_permits(clt); > - free_percpu(clt->pcpu_path); > - mutex_destroy(&clt->paths_ev_mutex); > - mutex_destroy(&clt->paths_mutex); > /* release callback will free clt in last put */ > device_unregister(&clt->dev); > }
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c index b159471a8959..fbce9cb87d08 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c @@ -2680,6 +2680,7 @@ static void rtrs_clt_dev_release(struct device *dev) struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess, dev); + free_percpu(clt->pcpu_path); kfree(clt); } @@ -2760,8 +2761,6 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num, err_dev: device_unregister(&clt->dev); err: - free_percpu(clt->pcpu_path); - kfree(clt); return ERR_PTR(err); }
Callback function rtrs_clt_dev_release() for put_device() calls kfree(clt) to free memory. We shouldn't call kfree(clt) again, and we can't use the clt after kfree too. Fixes: 6a98d71daea1 ("RDMA/rtrs: client: main functionality") Reported-by: Miaoqian Lin <linmq006@gmail.com> Signed-off-by: Jack Wang <jinpu.wang@ionos.com> --- drivers/infiniband/ulp/rtrs/rtrs-clt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)