Re: [PATCH] rpmsg: fix driver_override memory leak

[Mathieu Poirier]

On Tue, 16 Jun 2020 at 11:19, ClÃment Leger <cleger@xxxxxxxxx> wrote:
>
> Hi Mathieu,
>
> ----- On 16 Jun, 2020, at 19:10, Mathieu Poirier mathieu.poirier@xxxxxxxxxx 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@xxxxxxxxx>
> >> ---
> >>  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