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

PATCH In review Kouya Shimura 3 participants 7 messages Nov 28, 2012 – Nov 29, 2012

lore

Share & tools

lore

Kouya Shimura

author

2012-11-28 06:51 UTC #0 lore raw

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

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

Signed-off-by Kouya Shimura <[email protected]>

xen/arch/x86/mm/hap/hap.c +6 -0

diff -r 0049de3827bc xen/arch/x86/mm/hap/hap.c
@@ -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);
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2012-11-28 08:39 UTC #1 lore raw

Quoted 4 lines

Quoted 1 line
Quoted 1 line · Kouya Shimura
This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
Signed-off-by Kouya Shimura <[email protected]>

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

Kouya Shimura

author

2012-11-29 01:00 UTC #2 lore raw

Quoted 11 lines · Jan Beulich

Quoted 4 lines
Quoted 1 line
Quoted 1 line · Kouya Shimura
This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
Signed-off-by Kouya Shimura <[email protected]>
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

Jan Beulich

2012-11-29 07:26 UTC #3 lore raw

Quoted 7 lines

Quoted 1 line
Quoted 1 line · Kouya Shimura
Quoted 5 lines · Jan Beulich
Quoted 4 lines
Quoted 1 line
Quoted 1 line · Kouya Shimura
This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
Signed-off-by Kouya Shimura <[email protected]>

Quoted 8 lines

Quoted 3 lines
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.

Quoted 3 lines

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

Tim Deegan

2012-11-29 11:05 UTC #4 lore raw

Quoted 11 lines · Jan Beulich

Quoted 11 lines
Quoted 1 line
Quoted 1 line · Kouya Shimura
Quoted 8 lines · Jan Beulich
Quoted 4 lines
Quoted 1 line
Quoted 1 line · Kouya Shimura
This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
Signed-off-by Kouya Shimura <[email protected]>
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.

Quoted 6 lines

Quoted 4 lines
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.

Jan Beulich

2012-11-29 13:16 UTC #5 lore raw

Quoted 18 lines

Quoted 1 line
Quoted 1 line · Tim Deegan
Quoted 16 lines · Jan Beulich
Quoted 15 lines
Quoted 1 line
Quoted 1 line · Kouya Shimura
Quoted 8 lines · Jan Beulich
Quoted 4 lines
Quoted 1 line
Quoted 1 line · Kouya Shimura
This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
Signed-off-by Kouya Shimura <[email protected]>
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.

Quoted 5 lines

Quoted 1 line
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

Tim Deegan

2012-11-29 15:25 UTC #6 lore raw

Quoted 6 lines · Jan Beulich

Quoted 4 lines
Quoted 1 line
Quoted 1 line · Tim Deegan
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.

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

Signed-off-by Kouya Shimura <[email protected]>

Signed-off-by Tim Deegan <[email protected]>

xen/arch/x86/mm/hap/hap.c +3 -0

diff -r d89986111f0c xen/arch/x86/mm/hap/hap.c
@@ -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);
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel