[PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Kouya Shimura]

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

This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>

[-- Attachment #2: fix_hap_leak.patch --]
[-- Type: text/x-patch, Size: 457 bytes --]

diff -r 0049de3827bc xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c	Fri Nov 23 11:06:15 2012 +0000
+++ b/xen/arch/x86/mm/hap/hap.c	Wed Nov 28 15:21:32 2012 +0900
@@ -567,6 +567,12 @@ void hap_teardown(struct domain *d)
 
     d->arch.paging.mode &= ~PG_log_dirty;
 
+    if ( d->arch.hvm_domain.dirty_vram )
+    {
+        xfree(d->arch.hvm_domain.dirty_vram);
+        d->arch.hvm_domain.dirty_vram = NULL;
+    }
+
     paging_unlock(d);
 }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Jan Beulich]

>>> On 28.11.12 at 07:51, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
> This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
> 
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>

Wouldn't it be more consistent (and less redundant) to do this
through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
not, the conditional around the freeing/clearing is pointless.

Jan

Re: [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Kouya Shimura]

On 11/28/2012 05:39 PM, Jan Beulich wrote:
>>>> On 28.11.12 at 07:51, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
>> This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
>>
>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>
> Wouldn't it be more consistent (and less redundant) to do this
> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
> not, the conditional around the freeing/clearing is pointless.
>
> Jan
>

 From another point of view, it's consistent since it almost
copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
That is a sibling function of hap_teardown().
If it's not preferable, another cleanup patch should be made.

IMHO, I feel no good to remain a pointer to the freed memory
even though it never be dereferenced any more.
So it makes sense to check the pointer and set it NULL.

Thanks,
Kouya

Re: [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Jan Beulich]

>>> On 29.11.12 at 02:00, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
> On 11/28/2012 05:39 PM, Jan Beulich wrote:
>>>>> On 28.11.12 at 07:51, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
>>> This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
>>>
>>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>>
>> Wouldn't it be more consistent (and less redundant) to do this
>> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
>> not, the conditional around the freeing/clearing is pointless.
> 
>  From another point of view, it's consistent since it almost
> copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
> That is a sibling function of hap_teardown().
> If it's not preferable, another cleanup patch should be made.

Tim will have the final say here anyway. But my general opinion
is that copying existing code is not an excuse to also copy its
eventual deficiencies.

> IMHO, I feel no good to remain a pointer to the freed memory
> even though it never be dereferenced any more.
> So it makes sense to check the pointer and set it NULL.

I didn't say you shouldn't clear the pointer field. All I said is
that the conditional around the freeing and clearing is pointless.

Jan

Re: [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Tim Deegan]

At 07:26 +0000 on 29 Nov (1354174019), Jan Beulich wrote:
> >>> On 29.11.12 at 02:00, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
> > On 11/28/2012 05:39 PM, Jan Beulich wrote:
> >>>>> On 28.11.12 at 07:51, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
> >>> This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
> >>>
> >>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> >>
> >> Wouldn't it be more consistent (and less redundant) to do this
> >> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
> >> not, the conditional around the freeing/clearing is pointless.
> > 
> >  From another point of view, it's consistent since it almost
> > copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
> > That is a sibling function of hap_teardown().
> > If it's not preferable, another cleanup patch should be made.
> 
> Tim will have the final say here anyway.

I have taken the conditional out but left it as an explicit free(). 
It might not always be safe to call hap_track_dirty_vram(), even for
disabling, this late in the teardown.

Thanks for the fix!

Tim.

Re: [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Jan Beulich]

>>> On 29.11.12 at 12:05, Tim Deegan <tim@xen.org> wrote:
> At 07:26 +0000 on 29 Nov (1354174019), Jan Beulich wrote:
>> >>> On 29.11.12 at 02:00, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
>> > On 11/28/2012 05:39 PM, Jan Beulich wrote:
>> >>>>> On 28.11.12 at 07:51, Kouya Shimura <kouya@jp.fujitsu.com> wrote:
>> >>> This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
>> >>>
>> >>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>> >>
>> >> Wouldn't it be more consistent (and less redundant) to do this
>> >> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
>> >> not, the conditional around the freeing/clearing is pointless.
>> > 
>> >  From another point of view, it's consistent since it almost
>> > copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
>> > That is a sibling function of hap_teardown().
>> > If it's not preferable, another cleanup patch should be made.
>> 
>> Tim will have the final say here anyway.
> 
> I have taken the conditional out but left it as an explicit free(). 
> It might not always be safe to call hap_track_dirty_vram(), even for
> disabling, this late in the teardown.

So is this something we also want to have on 4.2 and 4.1?

Jan

Re: [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

[Tim Deegan]

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

At 13:16 +0000 on 29 Nov (1354194991), Jan Beulich wrote:
> >>> On 29.11.12 at 12:05, Tim Deegan <tim@xen.org> wrote:
> > I have taken the conditional out but left it as an explicit free(). 
> > It might not always be safe to call hap_track_dirty_vram(), even for
> > disabling, this late in the teardown.
> 
> So is this something we also want to have on 4.2 and 4.1?

Yes, I think so (after the customary delay).  It applies OK to 4.2; 
a backport to 4.1 is attached.

Tim.

[-- Attachment #2: y --]
[-- Type: text/plain, Size: 546 bytes --]

x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
Signed-off-by: Tim Deegan <tim@xen.org>

diff -r d89986111f0c xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c	Tue Nov 27 13:28:36 2012 +0100
+++ b/xen/arch/x86/mm/hap/hap.c	Thu Nov 29 15:22:33 2012 +0000
@@ -686,6 +686,9 @@ void hap_teardown(struct domain *d)
 
     d->arch.paging.mode &= ~PG_log_dirty;
 
+    xfree(d->arch.hvm_domain.dirty_vram);
+    d->arch.hvm_domain.dirty_vram = NULL;
+
     hap_unlock(d);
 }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel