[PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Md Haris Iqbal]

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.

Replace device_register with device_initialize and device_add so that
dev_set_name can be used appropriately.

Move mutex_destroy to release function so it can be called in alloc_clt err
path.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index b696aa4abae4..d20bad345eff 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2685,6 +2685,8 @@ static void rtrs_clt_dev_release(struct device *dev)
 	struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess,
 						 dev);
 
+	mutex_destroy(&clt->paths_ev_mutex);
+	mutex_destroy(&clt->paths_mutex);
 	kfree(clt);
 }
 
@@ -2714,6 +2716,8 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num,
 		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,43 +2734,41 @@ 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->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:
+err_del:
+	device_del(&clt->dev);
+err_put:
 	free_percpu(clt->pcpu_path);
-	kfree(clt);
+	put_device(&clt->dev);
 	return ERR_PTR(err);
 }
 
@@ -2774,9 +2776,10 @@ 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 */
+
+	/*
+	 * release callback will free clt and destroy mutexes in last put
+	 */
 	device_unregister(&clt->dev);
 }
 
-- 
2.25.1

[PATCH 2/2] RDMA/rtrs-clt: Move free_permit from free_clt to rtrs_clt_close

[Md Haris Iqbal]

Error path of rtrs_clt_open calls free_clt, where free_permit is called.
This is wrong since error path of rtrs_clt_open does not need to call
free_permit.

Also, moving free_permits call to rtrs_clt_close, makes it more aligned
with the call to alloc_permit in rtrs_clt_open.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index d20bad345eff..c2c860d0c56e 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2774,7 +2774,6 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num,
 
 static void free_clt(struct rtrs_clt_sess *clt)
 {
-	free_permits(clt);
 	free_percpu(clt->pcpu_path);
 
 	/*
@@ -2896,6 +2895,7 @@ void rtrs_clt_close(struct rtrs_clt_sess *clt)
 		rtrs_clt_destroy_path_files(clt_path, NULL);
 		kobject_put(&clt_path->kobj);
 	}
+	free_permits(clt);
 	free_clt(clt);
 }
 EXPORT_SYMBOL(rtrs_clt_close);
-- 
2.25.1

Re: [PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Jason Gunthorpe]

On Wed, Feb 02, 2022 at 04:08:54PM +0100, Md Haris Iqbal 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.
> 
> Replace device_register with device_initialize and device_add so that
> dev_set_name can be used appropriately.
> 
> Move mutex_destroy to release function so it can be called in alloc_clt err
> path.
> 
> Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
>  1 file changed, 20 insertions(+), 17 deletions(-)

These patches don't apply, please resend them

Jason

Re: [PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Haris Iqbal]

On Tue, Feb 8, 2022 at 5:48 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, Feb 02, 2022 at 04:08:54PM +0100, Md Haris Iqbal 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.
> >
> > Replace device_register with device_initialize and device_add so that
> > dev_set_name can be used appropriately.
> >
> > Move mutex_destroy to release function so it can be called in alloc_clt err
> > path.
> >
> > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
> >  1 file changed, 20 insertions(+), 17 deletions(-)
>
> These patches don't apply, please resend them

Sure. Will resend.

>
> Jason

Re: [PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Haris Iqbal]

On Tue, Feb 8, 2022 at 5:48 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, Feb 02, 2022 at 04:08:54PM +0100, Md Haris Iqbal 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.
> >
> > Replace device_register with device_initialize and device_add so that
> > dev_set_name can be used appropriately.
> >
> > Move mutex_destroy to release function so it can be called in alloc_clt err
> > path.
> >
> > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
> >  1 file changed, 20 insertions(+), 17 deletions(-)
>
> These patches don't apply, please resend them

Hi Jason,

I tried these 2 patches over wip/jgg-for-next (commit
2f1b2820b546c1eef07d15ed73db4177c0cf6d46) and it applies. Can you
check once more if there is some other issue? Thanks.

>
> Jason

Re: [PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Jason Gunthorpe]

On Wed, Feb 09, 2022 at 01:00:32PM +0100, Haris Iqbal wrote:
> On Tue, Feb 8, 2022 at 5:48 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Wed, Feb 02, 2022 at 04:08:54PM +0100, Md Haris Iqbal 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.
> > >
> > > Replace device_register with device_initialize and device_add so that
> > > dev_set_name can be used appropriately.
> > >
> > > Move mutex_destroy to release function so it can be called in alloc_clt err
> > > path.
> > >
> > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
> > >  1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > These patches don't apply, please resend them
> 
> Hi Jason,
> 
> I tried these 2 patches over wip/jgg-for-next (commit
> 2f1b2820b546c1eef07d15ed73db4177c0cf6d46) and it applies. Can you
> check once more if there is some other issue? Thanks.

It applied for you because you have the right 3 way information, I
don't. You need to generate and send patches against clean trees

Jason

[PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Md Haris Iqbal]

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.

Replace device_register with device_initialize and device_add so that
dev_set_name can be used appropriately.

Move mutex_destroy to release function so it can be called in alloc_clt err
path.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index b696aa4abae4..d20bad345eff 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2685,6 +2685,8 @@ static void rtrs_clt_dev_release(struct device *dev)
 	struct rtrs_clt_sess *clt = container_of(dev, struct rtrs_clt_sess,
 						 dev);
 
+	mutex_destroy(&clt->paths_ev_mutex);
+	mutex_destroy(&clt->paths_mutex);
 	kfree(clt);
 }
 
@@ -2714,6 +2716,8 @@ static struct rtrs_clt_sess *alloc_clt(const char *sessname, size_t paths_num,
 		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,43 +2734,41 @@ 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->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:
+err_del:
+	device_del(&clt->dev);
+err_put:
 	free_percpu(clt->pcpu_path);
-	kfree(clt);
+	put_device(&clt->dev);
 	return ERR_PTR(err);
 }
 
@@ -2774,9 +2776,10 @@ 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 */
+
+	/*
+	 * release callback will free clt and destroy mutexes in last put
+	 */
 	device_unregister(&clt->dev);
 }
 

base-commit: 2f1b2820b546c1eef07d15ed73db4177c0cf6d46
-- 
2.25.1

Re: [PATCH v3 1/2] RDMA/rtrs-clt: Fix possible double free in error case

[Jason Gunthorpe]

On Wed, Feb 09, 2022 at 04:14:24PM +0100, Md Haris Iqbal 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.
> 
> Replace device_register with device_initialize and device_add so that
> dev_set_name can be used appropriately.
> 
> Move mutex_destroy to release function so it can be called in alloc_clt err
> path.
> 
> Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 37 ++++++++++++++------------
>  1 file changed, 20 insertions(+), 17 deletions(-)

Fixes lines for both patches?

Jason