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

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

[Jack Wang]

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);
 	return ERR_PTR(err);
 }
 
-- 
2.25.1

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

[Haris Iqbal]

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
>

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

[Jason Gunthorpe]

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);
 }

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

[Haris Iqbal]

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);
>  }