Re: [Xen-devel] [PATCH 06 of 14] Don't lose track of the log dirty bitmap

[Tim Deegan <tim@xxxxxxx>]

At 16:11 -0500 on 23 Nov (1322064673), Andres Lagar-Cavilla wrote:
> hap_log_dirty_init unconditionally sets the top of the log dirty
> bitmap to INVALID_MFN. If there had been a bitmap allocated, it is
> then leaked, and the host crashes on an ASSERT when the domain is
> cleaned up. Fixing it here.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> diff -r e8f0709af2b7 -r e22610ef339a xen/arch/x86/mm/hap/hap.c
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -224,11 +224,18 @@ static void hap_clean_dirty_bitmap(struc
>  void hap_logdirty_init(struct domain *d)
>  {
>      struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
> -    if ( paging_mode_log_dirty(d) && dirty_vram )
> +    if ( paging_mode_log_dirty(d) )
>      {
> -        paging_log_dirty_disable(d);
> -        xfree(dirty_vram);
> -        dirty_vram = d->arch.hvm_domain.dirty_vram = NULL;
> +        if ( dirty_vram )
> +        {
> +            paging_log_dirty_disable(d);
> +            xfree(dirty_vram);
> +            dirty_vram = d->arch.hvm_domain.dirty_vram = NULL;
> +        } else {
> +            /* We still need to clean up the bitmap, because
> +             * init below will overwrite it */
> +            paging_free_log_dirty_bitmap(d);

That's not safe - in the particular case you're worried about there are
concurrent users of this bitmap.  I think the only sane way to deal with
this is to make sure the initialization of log_dirty.top to INVALID_MFN
happens once at start of day, and remove it from the functions that get
called mulitple times. 

Something like this:

diff -r f71ff2f7b604 xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c  Thu Nov 24 15:20:57 2011 +0000
+++ b/xen/arch/x86/mm/paging.c  Thu Nov 24 16:05:46 2011 +0000
@@ -595,7 +595,6 @@ void paging_log_dirty_init(struct domain
     d->arch.paging.log_dirty.enable_log_dirty = enable_log_dirty;
     d->arch.paging.log_dirty.disable_log_dirty = disable_log_dirty;
     d->arch.paging.log_dirty.clean_dirty_bitmap = clean_dirty_bitmap;
-    d->arch.paging.log_dirty.top = _mfn(INVALID_MFN);
 }
 
 /* This function fress log dirty bitmap resources. */
@@ -617,6 +616,11 @@ int paging_domain_init(struct domain *d,
 
     mm_lock_init(&d->arch.paging.lock);
 
+    /* This must be initialized separately from the rest of the
+     * log-dirty init code as that can be called more than once and we
+     * don't want to leak any active log-dirty bitmaps */
+    d->arch.paging.log_dirty.top = _mfn(INVALID_MFN);
+
     /* The order of the *_init calls below is important, as the later
      * ones may rewrite some common fields.  Shadow pagetables are the
      * default... */

Does that make sense to you?

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel