Re: [PATCH] drm/mgag200: fix memory leak

From: Archit Taneja <architt@codeaurora.org>
To: Dave Airlie <airlied@gmail.com>, Ingo Molnar <mingo@kernel.org>
Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
	David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	LKML <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Archit Taneja <archit@ti.com>
Subject: Re: [PATCH] drm/mgag200: fix memory leak
Date: Mon, 14 Sep 2015 20:53:57 +0530	[thread overview]
Message-ID: <55F6E68D.8070800@codeaurora.org> (raw)
In-Reply-To: <CAPM=9tyRJ-vWqxTMHeUXDm7+2RH8gtPwvx-rRsSOweB4CLw96w@mail.gmail.com>

Hi,

On 9/14/2015 3:35 PM, Dave Airlie wrote:
> (this time with correct email address).
>
> On 14 September 2015 at 20:04, Dave Airlie <airlied@gmail.com> wrote:
>>>
>>>> If drm_fb_helper_alloc_fbi() fails then we were directly returning
>>>> without freeing sysram. Also if drm_fb_helper_alloc_fbi() succeeds but
>>>> mgag200_framebuffer_init() fails then we were not releasing sysram and
>>>> we were not releasing fbi helper also.
>>>>
>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
>>>> ---
>>>>   drivers/gpu/drm/mgag200/mgag200_fb.c | 15 ++++++++++++---
>>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
>>>> index 87de15e..5fe476a 100644
>>>> --- a/drivers/gpu/drm/mgag200/mgag200_fb.c
>>>> +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
>>>> @@ -189,14 +189,16 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
>>>>                return -ENOMEM;
>>>>
>>>>        info = drm_fb_helper_alloc_fbi(helper);
>>>> -     if (IS_ERR(info))
>>>> -             return PTR_ERR(info);
>>>> +     if (IS_ERR(info)) {
>>>> +             ret = PTR_ERR(info);
>>>> +             goto err_alloc_fbi;
>>>> +     }
>>>>
>>>>        info->par = mfbdev;
>>>>
>>>>        ret = mgag200_framebuffer_init(dev, &mfbdev->mfb, &mode_cmd, gobj);
>>>>        if (ret)
>>>> -             return ret;
>>>> +             goto err_framebuffer_init;
>>>>
>>>>        mfbdev->sysram = sysram;
>>>>        mfbdev->size = size;
>>>> @@ -226,6 +228,13 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
>>>>        DRM_DEBUG_KMS("allocated %dx%d\n",
>>>>                      fb->width, fb->height);
>>>>        return 0;
>>>> +
>>>> +err_framebuffer_init:
>>>> +     drm_fb_helper_release_fbi(helper);
>>>> +
>>>> +err_alloc_fbi:
>>>> +     vfree(sysram);
>>>> +     return ret;
>>>>   }
>>>>
>>>>   static int mga_fbdev_destroy(struct drm_device *dev,
>>>
>>> There's a new regression: v4.3-rc1 crashes on bootup on non-supported hardware, if
>>> CONFIG_DRM_MGAG200=y (built into the kernel).
>>
>> Archit, I'm guessing this is some fallout from the fbdev changes.
>>
>> There is no reason we should need CONFIG_FB_LITTLE_ENDIAN I don't think.

It looks like the mgag200 driver load fails and we crash in the error 
handling path. drm_fb_helper_fini ends up being called twice. The second
call tries to free resources that were already free'd.

The erroneous path above should have existed even without the recent
fbdev helper changes. I'm not sure what's causing the driver load
to fail in the first place. It looks like it's failing at
register_framebuffer. Is it possible that we never tried running
this driver before with Big Endian set?

The patch below fixes the problem with the error path mentioned
above. Could we try this?

From: Archit Taneja <architt@codeaurora.org>
Date: Mon, 14 Sep 2015 20:11:43 +0530
Subject: [PATCH] drm/mgag200: Prevent calling drm_fb_helper_fini twice

mgag200_fbdev_init's error handling path calls drm_fb_helper_fini before
bailing out. The error handling path of mgag200_driver_load also ends
up calling drm_fb_helper_fini.

This results in drm_fb_helper_fini being called twice if the driver load
drm op fails somewhere in between.

Make only mgag200_driver_unload call drm_fb_helper_fini, remove the call
from mgag200_fbdev_init.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
  drivers/gpu/drm/mgag200/mgag200_fb.c | 8 ++------
  1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c 
b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 87de15e..6259b0a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -280,20 +280,16 @@ int mgag200_fbdev_init(struct mga_device *mdev)

  	ret = drm_fb_helper_single_add_all_connectors(&mfbdev->helper);
  	if (ret)
-		goto fini;
+		return ret;

  	/* disable all the possible outputs/crtcs before entering KMS mode */
  	drm_helper_disable_unused_functions(mdev->dev);

  	ret = drm_fb_helper_initial_config(&mfbdev->helper, bpp_sel);
  	if (ret)
-		goto fini;
+		return ret;

  	return 0;
-
-fini:
-	drm_fb_helper_fini(&mfbdev->helper);
-	return ret;
  }

  void mgag200_fbdev_fini(struct mga_device *mdev)



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
