Message-ID: <1643320865.7759489.1592327984836.JavaMail.zimbra@kalray.eu>
Date:   Tue, 16 Jun 2020 19:19:44 +0200 (CEST)
From:   Clément Leger <cleger@...ray.eu>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        linux-remoteproc <linux-remoteproc@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] rpmsg: fix driver_override memory leak

Hi Mathieu,

----- On 16 Jun, 2020, at 19:10, Mathieu Poirier mathieu.poirier@...aro.org 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 <cleger@...ray.eu>
>> ---
>>  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

Powered by blists - more mailing lists