[PATCH 6/9] drm/i915/gvt: Fix a memory leak in cmd_parser.c

[Wang, Zhi A]

The point is If the allocation is failed, a NULL is returned, then the old one is lost.

This problem hides because it passes va to krealloc(), but actually va == vgpu->reserve... stuff. If it passes directly vgpu->reserve stuff to krealloc(),

e.g. vgpu->reserve.. = krealloc(vgpu->reserve...), checkpatch.pl will directly throw an error.

-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On Behalf Of Zhenyu Wang
Sent: Monday, September 11, 2017 8:58 AM
To: Wang, Zhi A <zhi.a.wang at intel.com>
Cc: intel-gvt-dev at lists.freedesktop.org
Subject: Re: [PATCH 6/9] drm/i915/gvt: Fix a memory leak in cmd_parser.c

On 2017.09.11 12:44:10 +0800, Zhi Wang wrote:
> The pointer points to the original memory can never take the return 
> value of krealloc().
> 
> Signed-off-by: Zhi Wang <zhi.a.wang at intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/cmd_parser.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c 
> b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 2c0ccbb..22d33be 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -2620,14 +2620,16 @@ static int shadow_workload_ring_buffer(struct intel_vgpu_workload *workload)
>  	gma_top = workload->rb_start + guest_rb_size;
>  
>  	if (workload->rb_len > vgpu->reserve_ring_buffer_size[ring_id]) {
> -		void *va = vgpu->reserve_ring_buffer_va[ring_id];
> +		void *va, *p;
> +
>  		/* realloc the new ring buffer if needed */
> -		vgpu->reserve_ring_buffer_va[ring_id] =
> -			krealloc(va, workload->rb_len, GFP_KERNEL);
> -		if (!vgpu->reserve_ring_buffer_va[ring_id]) {
> +		va = vgpu->reserve_ring_buffer_va[ring_id];
> +		p = krealloc(va, workload->rb_len, GFP_KERNEL);
> +		if (!p) {
>  			gvt_vgpu_err("fail to alloc reserve ring buffer\n");
>  			return -ENOMEM;
>  		}
> +		vgpu->reserve_ring_buffer_va[ring_id] = p;
>  		vgpu->reserve_ring_buffer_size[ring_id] = workload->rb_len;
>  	}

It shouldn't be leaking as if new pointer is different than old, krealloc is supposed to free old ptr. So more clean to write might be

   void *va = krealloc(vgpu->reserve_ring_buffer_va[ring_id], ...);
   if (!va) {...}
   vgpu->reserve_ring_buffer_va[ring_id] = va;

but looks you're cleaning up this anyway.

--
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827