2020-06-11 19:22:29

by Clément Leger

Subject: [PATCH] rpmsg: fix driver_override memory leak

rpmsg_core allows to override driver using driver_override sysfs
attribute. When used, the sysfs store function will duplicate the user
provided string using kstrndup. However, when the rpdev is released,
the driver_override attribute is not freed. In order to have a
consistent allocation and release, use kstrdup in
rpmsg_chrdev_register_device and move it in rpmsg_core.c to avoid
header dependencies. Moreover, add a rpmsg_release_device function to
be called in device release. Drivers using rpmsg have been modified to
use this function and ensure there will be no more memory leak when
releasing rpmsg devices.
This was found with kmemleak while using remoteproc and virtio.

Signed-off-by: Clement Leger <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 1 +
drivers/rpmsg/qcom_smd.c | 1 +
drivers/rpmsg/rpmsg_core.c | 22 ++++++++++++++++++++++
drivers/rpmsg/rpmsg_internal.h | 15 ++-------------
drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
5 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 1995f5b3ea67..076997afc638 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1373,6 +1373,7 @@ static void qcom_glink_rpdev_release(struct device *dev)
struct glink_channel *channel = to_glink_channel(rpdev->ept);

channel->rpdev = NULL;
+ rpmsg_release_device(rpdev);
kfree(rpdev);
}

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 4abbeea782fa..f01174d0d4d9 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -1047,6 +1047,7 @@ static void qcom_smd_release_device(struct device *dev)
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
struct qcom_smd_device *qsdev = to_smd_device(rpdev);

+ rpmsg_release_device(rpdev);
kfree(qsdev);
}

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index a6361cad608b..31de89c81b27 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -554,6 +554,28 @@ int rpmsg_unregister_device(struct device *parent,
}
EXPORT_SYMBOL(rpmsg_unregister_device);

+void rpmsg_release_device(struct rpmsg_device *rpdev)
+{
+ kfree(rpdev->driver_override);
+}
+EXPORT_SYMBOL(rpmsg_release_device);
+
+/**
+ * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ *
+ * This function wraps rpmsg_register_device() preparing the rpdev for use as
+ * basis for the rpmsg chrdev.
+ */
+int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
+{
+ strcpy(rpdev->id.name, "rpmsg_chrdev");
+ rpdev->driver_override = kstrdup("rpmsg_chrdev", GFP_KERNEL);
+
+ return rpmsg_register_device(rpdev);
+}
+EXPORT_SYMBOL(rpmsg_chrdev_register_device);
+
/**
* __register_rpmsg_driver() - register an rpmsg driver with the rpmsg bus
* @rpdrv: pointer to a struct rpmsg_driver
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 3fc83cd50e98..043b28f912fd 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -75,19 +75,8 @@ int rpmsg_unregister_device(struct device *parent,
struct device *rpmsg_find_device(struct device *parent,
struct rpmsg_channel_info *chinfo);

-/**
- * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
- * @rpdev: prepared rpdev to be used for creating endpoints
- *
- * This function wraps rpmsg_register_device() preparing the rpdev for use as
- * basis for the rpmsg chrdev.
- */
-static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
-{
- strcpy(rpdev->id.name, "rpmsg_chrdev");
- rpdev->driver_override = "rpmsg_chrdev";
+int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev);

- return rpmsg_register_device(rpdev);
-}
+void rpmsg_release_device(struct rpmsg_device *rpdev);

#endif
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 07d4f3374098..af4ea6170f89 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -381,6 +381,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);

+ rpmsg_release_device(rpdev);
kfree(vch);
}

--
2.17.1

2020-06-16 17:12:37

by Mathieu Poirier

Subject: Re: [PATCH] rpmsg: fix driver_override memory leak

Hi Cl?ment,

On Thu, Jun 11, 2020 at 08:50:12PM +0200, Clement Leger wrote:
> rpmsg_core allows to override driver using driver_override sysfs
> attribute. When used, the sysfs store function will duplicate the user
> provided string using kstrndup. However, when the rpdev is released,
> the driver_override attribute is not freed. In order to have a
> consistent allocation and release, use kstrdup in
> rpmsg_chrdev_register_device and move it in rpmsg_core.c to avoid
> header dependencies. Moreover, add a rpmsg_release_device function to
> be called in device release. Drivers using rpmsg have been modified to
> use this function and ensure there will be no more memory leak when
> releasing rpmsg devices.
> This was found with kmemleak while using remoteproc and virtio.
>
> Signed-off-by: Clement Leger <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_native.c | 1 +
> drivers/rpmsg/qcom_smd.c | 1 +
> drivers/rpmsg/rpmsg_core.c | 22 ++++++++++++++++++++++
> drivers/rpmsg/rpmsg_internal.h | 15 ++-------------
> drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
> 5 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 1995f5b3ea67..076997afc638 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1373,6 +1373,7 @@ static void qcom_glink_rpdev_release(struct device *dev)
> struct glink_channel *channel = to_glink_channel(rpdev->ept);
>
> channel->rpdev = NULL;
> + rpmsg_release_device(rpdev);
> kfree(rpdev);
> }
>
> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> index 4abbeea782fa..f01174d0d4d9 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -1047,6 +1047,7 @@ static void qcom_smd_release_device(struct device *dev)
> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> struct qcom_smd_device *qsdev = to_smd_device(rpdev);
>
> + rpmsg_release_device(rpdev);
> kfree(qsdev);
> }
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index a6361cad608b..31de89c81b27 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -554,6 +554,28 @@ int rpmsg_unregister_device(struct device *parent,
> }
> EXPORT_SYMBOL(rpmsg_unregister_device);
>
> +void rpmsg_release_device(struct rpmsg_device *rpdev)
> +{
> + kfree(rpdev->driver_override);
> +}
> +EXPORT_SYMBOL(rpmsg_release_device);
> +
> +/**
> + * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + *
> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> + * basis for the rpmsg chrdev.
> + */
> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> +{
> + strcpy(rpdev->id.name, "rpmsg_chrdev");
> + rpdev->driver_override = kstrdup("rpmsg_chrdev", GFP_KERNEL);

Have you considered using devm_kstrdup() instead? Since the same rpdev is
available here and in field##_store(), proceeding that way would prevent the
need to add a new rpmsg_release_device() function. Depending on header
dependencies rpmsg_chrdev_register_device() may also be able to remain in
rpmsg_internal.h.

Thanks,
Mathieu

> +
> + return rpmsg_register_device(rpdev);
> +}
> +EXPORT_SYMBOL(rpmsg_chrdev_register_device);
> +
> /**
> * __register_rpmsg_driver() - register an rpmsg driver with the rpmsg bus
> * @rpdrv: pointer to a struct rpmsg_driver
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3fc83cd50e98..043b28f912fd 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -75,19 +75,8 @@ int rpmsg_unregister_device(struct device *parent,
> struct device *rpmsg_find_device(struct device *parent,
> struct rpmsg_channel_info *chinfo);
>
> -/**
> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> - * @rpdev: prepared rpdev to be used for creating endpoints
> - *
> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
> - * basis for the rpmsg chrdev.
> - */
> -static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> -{
> - strcpy(rpdev->id.name, "rpmsg_chrdev");
> - rpdev->driver_override = "rpmsg_chrdev";
> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev);
>
> - return rpmsg_register_device(rpdev);
> -}
> +void rpmsg_release_device(struct rpmsg_device *rpdev);
>
> #endif
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 07d4f3374098..af4ea6170f89 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -381,6 +381,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>
> + rpmsg_release_device(rpdev);
> kfree(vch);
> }
>
> --
> 2.17.1
>

2020-06-16 17:22:08

by Clément Leger

Subject: Re: [PATCH] rpmsg: fix driver_override memory leak

Hi Mathieu,

----- On 16 Jun, 2020, at 19:10, Mathieu Poirier [email protected] wrote:

> Hi Clément,
>
> On Thu, Jun 11, 2020 at 08:50:12PM +0200, Clement Leger wrote:
>> rpmsg_core allows to override driver using driver_override sysfs
>> attribute. When used, the sysfs store function will duplicate the user
>> provided string using kstrndup. However, when the rpdev is released,
>> the driver_override attribute is not freed. In order to have a
>> consistent allocation and release, use kstrdup in
>> rpmsg_chrdev_register_device and move it in rpmsg_core.c to avoid
>> header dependencies. Moreover, add a rpmsg_release_device function to
>> be called in device release. Drivers using rpmsg have been modified to
>> use this function and ensure there will be no more memory leak when
>> releasing rpmsg devices.
>> This was found with kmemleak while using remoteproc and virtio.
>>
>> Signed-off-by: Clement Leger <[email protected]>
>> ---
>> drivers/rpmsg/qcom_glink_native.c | 1 +
>> drivers/rpmsg/qcom_smd.c | 1 +
>> drivers/rpmsg/rpmsg_core.c | 22 ++++++++++++++++++++++
>> drivers/rpmsg/rpmsg_internal.h | 15 ++-------------
>> drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
>> 5 files changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c
>> b/drivers/rpmsg/qcom_glink_native.c
>> index 1995f5b3ea67..076997afc638 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -1373,6 +1373,7 @@ static void qcom_glink_rpdev_release(struct device *dev)
>> struct glink_channel *channel = to_glink_channel(rpdev->ept);
>>
>> channel->rpdev = NULL;
>> + rpmsg_release_device(rpdev);
>> kfree(rpdev);
>> }
>>
>> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
>> index 4abbeea782fa..f01174d0d4d9 100644
>> --- a/drivers/rpmsg/qcom_smd.c
>> +++ b/drivers/rpmsg/qcom_smd.c
>> @@ -1047,6 +1047,7 @@ static void qcom_smd_release_device(struct device *dev)
>> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
>> struct qcom_smd_device *qsdev = to_smd_device(rpdev);
>>
>> + rpmsg_release_device(rpdev);
>> kfree(qsdev);
>> }
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index a6361cad608b..31de89c81b27 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -554,6 +554,28 @@ int rpmsg_unregister_device(struct device *parent,
>> }
>> EXPORT_SYMBOL(rpmsg_unregister_device);
>>
>> +void rpmsg_release_device(struct rpmsg_device *rpdev)
>> +{
>> + kfree(rpdev->driver_override);
>> +}
>> +EXPORT_SYMBOL(rpmsg_release_device);
>> +
>> +/**
>> + * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + *
>> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> + * basis for the rpmsg chrdev.
>> + */
>> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>> +{
>> + strcpy(rpdev->id.name, "rpmsg_chrdev");
>> + rpdev->driver_override = kstrdup("rpmsg_chrdev", GFP_KERNEL);
>
> Have you considered using devm_kstrdup() instead? Since the same rpdev is
> available here and in field##_store(), proceeding that way would prevent the
> need to add a new rpmsg_release_device() function. Depending on header
> dependencies rpmsg_chrdev_register_device() may also be able to remain in
> rpmsg_internal.h.

Indeed, using devm_kstrdup would be better. Regarding the use of kstrdup in
headers, I only found a really really few occurences of such usage in the
whole kernel. If you think it's ok, I can go go with it though.

Thanks,

Clément

>
> Thanks,
> Mathieu
>
>> +
>> + return rpmsg_register_device(rpdev);
>> +}
>> +EXPORT_SYMBOL(rpmsg_chrdev_register_device);
>> +
>> /**
>> * __register_rpmsg_driver() - register an rpmsg driver with the rpmsg bus
>> * @rpdrv: pointer to a struct rpmsg_driver
>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index 3fc83cd50e98..043b28f912fd 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -75,19 +75,8 @@ int rpmsg_unregister_device(struct device *parent,
>> struct device *rpmsg_find_device(struct device *parent,
>> struct rpmsg_channel_info *chinfo);
>>
>> -/**
>> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>> - * @rpdev: prepared rpdev to be used for creating endpoints
>> - *
>> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> - * basis for the rpmsg chrdev.
>> - */
>> -static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>> -{
>> - strcpy(rpdev->id.name, "rpmsg_chrdev");
>> - rpdev->driver_override = "rpmsg_chrdev";
>> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev);
>>
>> - return rpmsg_register_device(rpdev);
>> -}
>> +void rpmsg_release_device(struct rpmsg_device *rpdev);
>>
>> #endif
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 07d4f3374098..af4ea6170f89 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -381,6 +381,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
>> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
>> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>>
>> + rpmsg_release_device(rpdev);
>> kfree(vch);
>> }
>>
>> --
>> 2.17.1

2020-06-16 19:18:11

by Mathieu Poirier

Subject: Re: [PATCH] rpmsg: fix driver_override memory leak

On Tue, 16 Jun 2020 at 11:19, Clément Leger <[email protected]> wrote:
>
> Hi Mathieu,
>
> ----- On 16 Jun, 2020, at 19:10, Mathieu Poirier [email protected] wrote:
>
> > Hi Clément,
> >
> > On Thu, Jun 11, 2020 at 08:50:12PM +0200, Clement Leger wrote:
> >> rpmsg_core allows to override driver using driver_override sysfs
> >> attribute. When used, the sysfs store function will duplicate the user
> >> provided string using kstrndup. However, when the rpdev is released,
> >> the driver_override attribute is not freed. In order to have a
> >> consistent allocation and release, use kstrdup in
> >> rpmsg_chrdev_register_device and move it in rpmsg_core.c to avoid
> >> header dependencies. Moreover, add a rpmsg_release_device function to
> >> be called in device release. Drivers using rpmsg have been modified to
> >> use this function and ensure there will be no more memory leak when
> >> releasing rpmsg devices.
> >> This was found with kmemleak while using remoteproc and virtio.
> >>
> >> Signed-off-by: Clement Leger <[email protected]>
> >> ---
> >> drivers/rpmsg/qcom_glink_native.c | 1 +
> >> drivers/rpmsg/qcom_smd.c | 1 +
> >> drivers/rpmsg/rpmsg_core.c | 22 ++++++++++++++++++++++
> >> drivers/rpmsg/rpmsg_internal.h | 15 ++-------------
> >> drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
> >> 5 files changed, 27 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/qcom_glink_native.c
> >> b/drivers/rpmsg/qcom_glink_native.c
> >> index 1995f5b3ea67..076997afc638 100644
> >> --- a/drivers/rpmsg/qcom_glink_native.c
> >> +++ b/drivers/rpmsg/qcom_glink_native.c
> >> @@ -1373,6 +1373,7 @@ static void qcom_glink_rpdev_release(struct device *dev)
> >> struct glink_channel *channel = to_glink_channel(rpdev->ept);
> >>
> >> channel->rpdev = NULL;
> >> + rpmsg_release_device(rpdev);
> >> kfree(rpdev);
> >> }
> >>
> >> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> >> index 4abbeea782fa..f01174d0d4d9 100644
> >> --- a/drivers/rpmsg/qcom_smd.c
> >> +++ b/drivers/rpmsg/qcom_smd.c
> >> @@ -1047,6 +1047,7 @@ static void qcom_smd_release_device(struct device *dev)
> >> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> >> struct qcom_smd_device *qsdev = to_smd_device(rpdev);
> >>
> >> + rpmsg_release_device(rpdev);
> >> kfree(qsdev);
> >> }
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> >> index a6361cad608b..31de89c81b27 100644
> >> --- a/drivers/rpmsg/rpmsg_core.c
> >> +++ b/drivers/rpmsg/rpmsg_core.c
> >> @@ -554,6 +554,28 @@ int rpmsg_unregister_device(struct device *parent,
> >> }
> >> EXPORT_SYMBOL(rpmsg_unregister_device);
> >>
> >> +void rpmsg_release_device(struct rpmsg_device *rpdev)
> >> +{
> >> + kfree(rpdev->driver_override);
> >> +}
> >> +EXPORT_SYMBOL(rpmsg_release_device);
> >> +
> >> +/**
> >> + * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> >> + * @rpdev: prepared rpdev to be used for creating endpoints
> >> + *
> >> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
> >> + * basis for the rpmsg chrdev.
> >> + */
> >> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> >> +{
> >> + strcpy(rpdev->id.name, "rpmsg_chrdev");
> >> + rpdev->driver_override = kstrdup("rpmsg_chrdev", GFP_KERNEL);
> >
> > Have you considered using devm_kstrdup() instead? Since the same rpdev is
> > available here and in field##_store(), proceeding that way would prevent the
> > need to add a new rpmsg_release_device() function. Depending on header
> > dependencies rpmsg_chrdev_register_device() may also be able to remain in
> > rpmsg_internal.h.
>
> Indeed, using devm_kstrdup would be better. Regarding the use of kstrdup in
> headers, I only found a really really few occurences of such usage in the
> whole kernel. If you think it's ok, I can go go with it though.

I don't see an issue with using devm_kstrdup() in a header file.

>
> Thanks,
>
> Clément
>
> >
> > Thanks,
> > Mathieu
> >
> >> +
> >> + return rpmsg_register_device(rpdev);
> >> +}
> >> +EXPORT_SYMBOL(rpmsg_chrdev_register_device);
> >> +
> >> /**
> >> * __register_rpmsg_driver() - register an rpmsg driver with the rpmsg bus
> >> * @rpdrv: pointer to a struct rpmsg_driver
> >> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> >> index 3fc83cd50e98..043b28f912fd 100644
> >> --- a/drivers/rpmsg/rpmsg_internal.h
> >> +++ b/drivers/rpmsg/rpmsg_internal.h
> >> @@ -75,19 +75,8 @@ int rpmsg_unregister_device(struct device *parent,
> >> struct device *rpmsg_find_device(struct device *parent,
> >> struct rpmsg_channel_info *chinfo);
> >>
> >> -/**
> >> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> >> - * @rpdev: prepared rpdev to be used for creating endpoints
> >> - *
> >> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
> >> - * basis for the rpmsg chrdev.
> >> - */
> >> -static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
> >> -{
> >> - strcpy(rpdev->id.name, "rpmsg_chrdev");
> >> - rpdev->driver_override = "rpmsg_chrdev";
> >> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev);
> >>
> >> - return rpmsg_register_device(rpdev);
> >> -}
> >> +void rpmsg_release_device(struct rpmsg_device *rpdev);
> >>
> >> #endif
> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> index 07d4f3374098..af4ea6170f89 100644
> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >> @@ -381,6 +381,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
> >> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> >> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> >>
> >> + rpmsg_release_device(rpdev);
> >> kfree(vch);
> >> }
> >>
> >> --
> >> 2.17.1

2020-06-16 19:59:06

by Clément Leger

Subject: Re: [PATCH] rpmsg: fix driver_override memory leak

----- On 16 Jun, 2020, at 21:13, Mathieu Poirier [email protected] wrote:

> On Tue, 16 Jun 2020 at 11:19, Clément Leger <[email protected]> wrote:
>>
>> Hi Mathieu,
>>
>> ----- On 16 Jun, 2020, at 19:10, Mathieu Poirier [email protected]
>> wrote:
>>
>> > Hi Clément,
>> >
>> > On Thu, Jun 11, 2020 at 08:50:12PM +0200, Clement Leger wrote:
>> >> rpmsg_core allows to override driver using driver_override sysfs
>> >> attribute. When used, the sysfs store function will duplicate the user
>> >> provided string using kstrndup. However, when the rpdev is released,
>> >> the driver_override attribute is not freed. In order to have a
>> >> consistent allocation and release, use kstrdup in
>> >> rpmsg_chrdev_register_device and move it in rpmsg_core.c to avoid
>> >> header dependencies. Moreover, add a rpmsg_release_device function to
>> >> be called in device release. Drivers using rpmsg have been modified to
>> >> use this function and ensure there will be no more memory leak when
>> >> releasing rpmsg devices.
>> >> This was found with kmemleak while using remoteproc and virtio.
>> >>
>> >> Signed-off-by: Clement Leger <[email protected]>
>> >> ---
>> >> drivers/rpmsg/qcom_glink_native.c | 1 +
>> >> drivers/rpmsg/qcom_smd.c | 1 +
>> >> drivers/rpmsg/rpmsg_core.c | 22 ++++++++++++++++++++++
>> >> drivers/rpmsg/rpmsg_internal.h | 15 ++-------------
>> >> drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
>> >> 5 files changed, 27 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/rpmsg/qcom_glink_native.c
>> >> b/drivers/rpmsg/qcom_glink_native.c
>> >> index 1995f5b3ea67..076997afc638 100644
>> >> --- a/drivers/rpmsg/qcom_glink_native.c
>> >> +++ b/drivers/rpmsg/qcom_glink_native.c
>> >> @@ -1373,6 +1373,7 @@ static void qcom_glink_rpdev_release(struct device *dev)
>> >> struct glink_channel *channel = to_glink_channel(rpdev->ept);
>> >>
>> >> channel->rpdev = NULL;
>> >> + rpmsg_release_device(rpdev);
>> >> kfree(rpdev);
>> >> }
>> >>
>> >> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
>> >> index 4abbeea782fa..f01174d0d4d9 100644
>> >> --- a/drivers/rpmsg/qcom_smd.c
>> >> +++ b/drivers/rpmsg/qcom_smd.c
>> >> @@ -1047,6 +1047,7 @@ static void qcom_smd_release_device(struct device *dev)
>> >> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
>> >> struct qcom_smd_device *qsdev = to_smd_device(rpdev);
>> >>
>> >> + rpmsg_release_device(rpdev);
>> >> kfree(qsdev);
>> >> }
>> >>
>> >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> >> index a6361cad608b..31de89c81b27 100644
>> >> --- a/drivers/rpmsg/rpmsg_core.c
>> >> +++ b/drivers/rpmsg/rpmsg_core.c
>> >> @@ -554,6 +554,28 @@ int rpmsg_unregister_device(struct device *parent,
>> >> }
>> >> EXPORT_SYMBOL(rpmsg_unregister_device);
>> >>
>> >> +void rpmsg_release_device(struct rpmsg_device *rpdev)
>> >> +{
>> >> + kfree(rpdev->driver_override);
>> >> +}
>> >> +EXPORT_SYMBOL(rpmsg_release_device);
>> >> +
>> >> +/**
>> >> + * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>> >> + * @rpdev: prepared rpdev to be used for creating endpoints
>> >> + *
>> >> + * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> >> + * basis for the rpmsg chrdev.
>> >> + */
>> >> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>> >> +{
>> >> + strcpy(rpdev->id.name, "rpmsg_chrdev");
>> >> + rpdev->driver_override = kstrdup("rpmsg_chrdev", GFP_KERNEL);
>> >
>> > Have you considered using devm_kstrdup() instead? Since the same rpdev is
>> > available here and in field##_store(), proceeding that way would prevent the
>> > need to add a new rpmsg_release_device() function. Depending on header
>> > dependencies rpmsg_chrdev_register_device() may also be able to remain in
>> > rpmsg_internal.h.
>>
>> Indeed, using devm_kstrdup would be better. Regarding the use of kstrdup in
>> headers, I only found a really really few occurences of such usage in the
>> whole kernel. If you think it's ok, I can go go with it though.
>
> I don't see an issue with using devm_kstrdup() in a header file.

Ok, anyway after looking more thoroughly in using devm_kstrndup, it will
be possible to make the modification really localized in sysfs store only.
I just have to add devm_kstrndup which does not exists yet.

>
>>
>> Thanks,
>>
>> Clément
>>
>> >
>> > Thanks,
>> > Mathieu
>> >
>> >> +
>> >> + return rpmsg_register_device(rpdev);
>> >> +}
>> >> +EXPORT_SYMBOL(rpmsg_chrdev_register_device);
>> >> +
>> >> /**
>> >> * __register_rpmsg_driver() - register an rpmsg driver with the rpmsg bus
>> >> * @rpdrv: pointer to a struct rpmsg_driver
>> >> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> >> index 3fc83cd50e98..043b28f912fd 100644
>> >> --- a/drivers/rpmsg/rpmsg_internal.h
>> >> +++ b/drivers/rpmsg/rpmsg_internal.h
>> >> @@ -75,19 +75,8 @@ int rpmsg_unregister_device(struct device *parent,
>> >> struct device *rpmsg_find_device(struct device *parent,
>> >> struct rpmsg_channel_info *chinfo);
>> >>
>> >> -/**
>> >> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>> >> - * @rpdev: prepared rpdev to be used for creating endpoints
>> >> - *
>> >> - * This function wraps rpmsg_register_device() preparing the rpdev for use as
>> >> - * basis for the rpmsg chrdev.
>> >> - */
>> >> -static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)
>> >> -{
>> >> - strcpy(rpdev->id.name, "rpmsg_chrdev");
>> >> - rpdev->driver_override = "rpmsg_chrdev";
>> >> +int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev);
>> >>
>> >> - return rpmsg_register_device(rpdev);
>> >> -}
>> >> +void rpmsg_release_device(struct rpmsg_device *rpdev);
>> >>
>> >> #endif
>> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> >> index 07d4f3374098..af4ea6170f89 100644
>> >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> >> @@ -381,6 +381,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
>> >> struct rpmsg_device *rpdev = to_rpmsg_device(dev);
>> >> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>> >>
>> >> + rpmsg_release_device(rpdev);
>> >> kfree(vch);
>> >> }
>> >>
>> >> --
> > >> 2.17.1