On Wed, Nov 29, 2017 at 07:16:43PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote:
> > On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> > > Fix the leak of the CRTC structure in the failure paths of
> > > armada_drm_crtc_create().
> > >
> > > Signed-off-by: Russell King <rmk+ker...@armlinux.org.uk>
> > > ---
> > > drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
> > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> > > b/drivers/gpu/drm/armada/armada_crtc.c
> > > index 2e065facdce7..844d488b6654 100644
> > > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct
> > > drm_device *drm, struct device *dev,
> > > dcrtc->crtc.port = port;
> > >
> > > primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > - if (!primary)
> > > + if (!primary) {
> > > + kfree(dcrtc);
> > > return -ENOMEM;
> > > + }
> > >
> > > ret = armada_drm_plane_init(primary);
> > > if (ret) {
> > > + kfree(dcrtc);
> > > kfree(primary);
> > > return ret;
> > > }
> > > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct
> > > drm_device *drm, struct device *dev,
> > > NULL,
> > > DRM_PLANE_TYPE_PRIMARY, NULL);
> > > if (ret) {
> > > + kfree(dcrtc);
> > > kfree(primary);
> > > return ret;
> > > }
> > >
> > > ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> > > &armada_crtc_funcs, NULL);
> > > - if (ret)
> > > + if (ret) {
> > > + kfree(dcrtc);
> > > goto err_crtc_init;
> >
> > The mixed use of cleaning up at every exit and the lone label is a bit
> > messy and
> > prone to bugs such as the one you're fixing. How about you label everything
> > and then just clean up once?
>
> It's not that simple - see alternative patch below. How we clean
> stuff up depends on how far through the initialisation we are, and
> that doesn't lend itself to a sequential cleanup methodology.
It can be that simple, depending on how you setup your initialization sequence.
For instance, if armada_plane_init did the allocation as well as
universal_plane_init (like armada_overlay_plane_create already does), then you
wouldn't need to worry about kfree(primary) at all.
>
> > It also seems like you're missing cleanup if armada_overlay_plane_create()
> > fails?
>
> It's not missing. At that point, the CRTC has been registered, and any
> failure will be cleaned up by the drm_mode_config_cleanup() in the
> error paths in armada_drm_bind() in armada_drv.c
>
> It's rather non-obvious because it uses the ->destroy callbacks.
Got it, thanks!
Sean
>
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> b/drivers/gpu/drm/armada/armada_crtc.c
> index d987198a2ac6..79ce877bf45f 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -1225,17 +1225,13 @@ static int armada_drm_crtc_create(struct drm_device
> *drm, struct device *dev,
>
> ret = devm_request_irq(dev, irq, armada_drm_irq, 0, "armada_drm_crtc",
> dcrtc);
> - if (ret < 0) {
> - kfree(dcrtc);
> - return ret;
> - }
> + if (ret < 0)
> + goto err_crtc;
>
> if (dcrtc->variant->init) {
> ret = dcrtc->variant->init(dcrtc, dev);
> - if (ret) {
> - kfree(dcrtc);
> - return ret;
> - }
> + if (ret)
> + goto err_crtc;
> }
>
> /* Ensure AXI pipeline is enabled */
> @@ -1247,15 +1243,14 @@ static int armada_drm_crtc_create(struct drm_device
> *drm, struct device *dev,
>
> primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> if (!primary) {
> - kfree(dcrtc);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto err_crtc;
> }
>
> ret = armada_drm_plane_init(primary);
> if (ret) {
> - kfree(dcrtc);
> kfree(primary);
> - return ret;
> + goto err_crtc;
> }
>
> ret = drm_universal_plane_init(drm, &primary->base, 0,
> @@ -1265,17 +1260,14 @@ static int armada_drm_crtc_create(struct drm_device
> *drm, struct device *dev,
> NULL,
> DRM_PLANE_TYPE_PRIMARY, NULL);
> if (ret) {
> - kfree(dcrtc);
> kfree(primary);
> - return ret;
> + goto err_crtc;
> }
>
> ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> &armada_crtc_funcs, NULL);
> - if (ret) {
> - kfree(dcrtc);
> + if (ret)
> goto err_crtc_init;
> - }
>
> drm_crtc_helper_add(&dcrtc->crtc, &armada_crtc_helper_funcs);
>
> @@ -1288,6 +1280,9 @@ static int armada_drm_crtc_create(struct drm_device
> *drm, struct device *dev,
>
> err_crtc_init:
> primary->base.funcs->destroy(&primary->base);
> +err_crtc:
> + kfree(dcrtc);
> +
> return ret;
> }
>
>
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
