Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 16 Jun 2020 11:10:11 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Clement Leger <cleger@kalray.eu>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Ohad Ben-Cohen <ohad@wizery.com>,
        linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rpmsg: fix driver_override memory leak
Message-ID: <20200616171011.GA461427@xps15>
References: <20200611185012.23815-1-cleger@kalray.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200611185012.23815-1-cleger@kalray.eu>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@kalray.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.

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
>