Re: [Xen-devel] [PATCH] x86/hap: fix race condition between ENABLE_LOGDIRTY and track_dirty_vram hypercall

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 12:59 -0500 on 03 Dec (1354539567), Robert Phillips wrote:
> > Robert, in your patch you do wrap this all in the paging_lock, but then
> > unlock to call various enable and disable routines.  Is there a version
> > of this race condition there, where some other CPU might call
> > LOG_DIRTY_ENABLE while you've temporarily dropped the lock?
> 
> My proposed patch does not modify the problematic locking code so, 
> unfortunately, it preserves the race condition that Kouya Shimura 
> has discovered.  
> 
> I question whether his proposed patch would be suitable for the 
> multiple frame buffer situation that my proposed patch addresses.
> It is possible that a guest might be updating its frame buffers when 
> live migration starts, and the same race would result.
> 
> I think the domain.arch.paging.log_dirty  function pointers are problematic.
> They are modified and executed without benefit of locking.
> 
> I am uncomfortable with adding another lock.
> 
> I will look at updating my patch to avoid the race and will (hopefully) 
> avoid adding another lock.

Thanks.  I think the paging_lock can probably cover everything we need
here.  These are toolstack operations and should be fairly rare, and HAP
can do most of its work without the paging_lock.

Also, in the next version can you please update this section:

+int hap_track_dirty_vram(struct domain *d,
+                         unsigned long begin_pfn,
+                         unsigned long nr,
+                         XEN_GUEST_HANDLE_64(uint8) guest_dirty_bitmap)
+{
+    long rc = 0;
+    dv_dirty_vram_t *dirty_vram;
+       
+    paging_lock(d);
+    dirty_vram = d->arch.hvm_domain.dirty_vram;
+    if ( nr )
+    {
+        dv_range_t *range = NULL;
+        int size = ( nr + BITS_PER_LONG - 1 ) & ~( BITS_PER_LONG - 1 );
+        uint8_t dirty_bitmap[size];

not to allocate a guest-specified amount of stack memory.  This is one
of the things recently found and fixed in the existing code as XSA-27.
http://xenbits.xen.org/hg/staging/xen-4.1-testing.hg/rev/53ef1f35a0f8

Cheers,

Tim.

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