[PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Rakib Mullick]

A call to i915_add_request() has been made in function i915_gem_busy_ioctl(). i915_add_request can fail,
so in it's exit path previously allocated memory needs to be freed.

Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
---

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d18b07a..2dc24ab 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3512,9 +3512,11 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 			 * so emit a request to do so.
 			 */
 			request = kzalloc(sizeof(*request), GFP_KERNEL);
-			if (request)
+			if (request) {
 				ret = i915_add_request(obj->ring, NULL, request);
-			else
+				if (ret)
+					kfree(request);
+			} else
 				ret = -ENOMEM;
 		}
 

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Daniel Vetter]

On Wed, Nov 16, 2011 at 12:49:28AM +0600, Rakib Mullick wrote:
> 
> A call to i915_add_request() has been made in function i915_gem_busy_ioctl(). i915_add_request can fail,
> so in it's exit path previously allocated memory needs to be freed.
> 
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>

Indeed, nice catch (albeit totally unlikely to be hit, because the error
only happens when the gpu ceases to progress in the ring, so imo not
stable material). Keith, please pick this up for fixes, thanks.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:

> Indeed, nice catch (albeit totally unlikely to be hit, because the error
> only happens when the gpu ceases to progress in the ring, so imo not
> stable material). Keith, please pick this up for fixes, thanks.

It's already there and queued for airlied :-)

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Rakib Mullick]

On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard <keithp@keithp.com> wrote:
> On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> Indeed, nice catch (albeit totally unlikely to be hit, because the error
>> only happens when the gpu ceases to progress in the ring, so imo not
>> stable material). Keith, please pick this up for fixes, thanks.
>
> It's already there and queued for airlied :-)
>
Thank you guys for reviewing and taking the patch.

Now, while I was looking at the uses of i915_add_request(), I found
the following code :

                        ret = i915_gem_flush_ring(ring, 0,
I915_GEM_GPU_DOMAINS);
                        request = kzalloc(sizeof(*request), GFP_KERNEL);
                        if (ret || request == NULL ||
                            i915_add_request(ring, NULL, request))
                            kfree(request);

>From above code, we might ended up by calling kfree(request) without
allocating request. Though, it's unlikely cause if request is NULL
then BUG_ON will be hit in i915_add_request(). So, to unify the callee
uses of i915_add_request(), I'm proposing the following patch. Please
let me know what do you guys think. If you guys agree, I can sent a
formal patch.

Index: work/modi.linux-3.1.1/drivers/gpu/drm/i915/i915_gem.c
===================================================================
--- work.orig/modi.linux-3.1.1/drivers/gpu/drm/i915/i915_gem.c
+++ work/modi.linux-3.1.1/drivers/gpu/drm/i915/i915_gem.c
@@ -1736,8 +1736,6 @@ i915_add_request(struct intel_ring_buffe
 	int was_empty;
 	int ret;

-	BUG_ON(request == NULL);
-
 	ret = ring->add_request(ring, &seqno);
 	if (ret)
 	    return ret;
@@ -2002,9 +2000,11 @@ i915_gem_retire_work_handler(struct work
 			ret = i915_gem_flush_ring(ring,
 						  0, I915_GEM_GPU_DOMAINS);
 			request = kzalloc(sizeof(*request), GFP_KERNEL);
-			if (ret || request == NULL ||
-			    i915_add_request(ring, NULL, request))
-			    kfree(request);
+			if (request) {
+				ret = i915_add_request(ring, NULL, request);
+				if (ret)
+					kfree(request);
+			}
 		}

 		idle &= list_empty(&ring->request_list);


Thanks,
Rakib

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Chris Wilson]

On Sat, 26 Nov 2011 10:44:17 +0600, Rakib Mullick <rakib.mullick@gmail.com> wrote:
> On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard <keithp@keithp.com> wrote:
> > On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >> Indeed, nice catch (albeit totally unlikely to be hit, because the error
> >> only happens when the gpu ceases to progress in the ring, so imo not
> >> stable material). Keith, please pick this up for fixes, thanks.
> >
> > It's already there and queued for airlied :-)
> >
> Thank you guys for reviewing and taking the patch.
> 
> Now, while I was looking at the uses of i915_add_request(), I found
> the following code :
> 
>                         ret = i915_gem_flush_ring(ring, 0,
> I915_GEM_GPU_DOMAINS);
>                         request = kzalloc(sizeof(*request), GFP_KERNEL);
>                         if (ret || request == NULL ||
>                             i915_add_request(ring, NULL, request))
>                             kfree(request);
> 
> From above code, we might ended up by calling kfree(request) without
> allocating request. Though, it's unlikely cause if request is NULL
> then BUG_ON will be hit in i915_add_request(). So, to unify the callee
> uses of i915_add_request(), I'm proposing the following patch. Please
> let me know what do you guys think. If you guys agree, I can sent a
> formal patch.

kfree(NULL) is permitted and taken advantage of here along with the
short-circuiting behaviour of '||'.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Rakib Mullick]

On Sat, Nov 26, 2011 at 4:57 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, 26 Nov 2011 10:44:17 +0600, Rakib Mullick <rakib.mullick@gmail.com> wrote:
>> On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard <keithp@keithp.com> wrote:
>> > On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >
>> >> Indeed, nice catch (albeit totally unlikely to be hit, because the error
>> >> only happens when the gpu ceases to progress in the ring, so imo not
>> >> stable material). Keith, please pick this up for fixes, thanks.
>> >
>> > It's already there and queued for airlied :-)
>> >
>> Thank you guys for reviewing and taking the patch.
>>
>> Now, while I was looking at the uses of i915_add_request(), I found
>> the following code :
>>
>>                         ret = i915_gem_flush_ring(ring, 0,
>> I915_GEM_GPU_DOMAINS);
>>                         request = kzalloc(sizeof(*request), GFP_KERNEL);
>>                         if (ret || request == NULL ||
>>                             i915_add_request(ring, NULL, request))
>>                             kfree(request);
>>
>> From above code, we might ended up by calling kfree(request) without
>> allocating request. Though, it's unlikely cause if request is NULL
>> then BUG_ON will be hit in i915_add_request(). So, to unify the callee
>> uses of i915_add_request(), I'm proposing the following patch. Please
>> let me know what do you guys think. If you guys agree, I can sent a
>> formal patch.
>
> kfree(NULL) is permitted and taken advantage of here along with the
> short-circuiting behaviour of '||'.

Yes, no real problem with current code. I was just thinking from code
cleanup's pov. Is BUG_ON really needed in i915_add_request() ?

thanks,
Rakib

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Chris Wilson]

On Sat, 26 Nov 2011 22:29:12 +0600, Rakib Mullick <rakib.mullick@gmail.com> wrote:
> Yes, no real problem with current code. I was just thinking from code
> cleanup's pov. Is BUG_ON really needed in i915_add_request() ?

No, just documentation as a reminder that the request should be
preallocated, ideally so that we can fail gracefully without touching
hardware and leaving it in an inconsistent state wrt to our bookkeeping.
(This is more apparent in the overlay code which could hang the
chip/driver if we hit a malloc error too late.)

The BUG_ON has certainly outlived its usefulness.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().

[Rakib Mullick]

On Sat, Nov 26, 2011 at 10:53 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, 26 Nov 2011 22:29:12 +0600, Rakib Mullick <rakib.mullick@gmail.com> wrote:
>> Yes, no real problem with current code. I was just thinking from code
>> cleanup's pov. Is BUG_ON really needed in i915_add_request() ?
>
> No, just documentation as a reminder that the request should be
> preallocated, ideally so that we can fail gracefully without touching
> hardware and leaving it in an inconsistent state wrt to our bookkeeping.
> (This is more apparent in the overlay code which could hang the
> chip/driver if we hit a malloc error too late.)
>
> The BUG_ON has certainly outlived its usefulness.

Actually, I'm not seeing how BUG_ON could trigger (though, I've
wrongly mentioned in previous thread, if request == NULL, BUG_ON could
trigger), it's usefulness will never come into action. Other callers
of i915_add_request also makes sure that, it gets called only if
(request).

Although, kfree(NULL) is permitted, we shouldn't use it unnecessarily.
Anyway, since the issue is not a big deal and no real bug, it could be
droped.

Thanks,
Rakib