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

[Robert Phillips <robert.phillips@xxxxxxxxxx>]

Tim,

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Thursday, November 29, 2012 10:41 AM
> To: Kouya Shimura
> Cc: Robert Phillips; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH] x86/hap: fix race condition between
> ENABLE_LOGDIRTY and track_dirty_vram hypercall
> 
> At 16:02 +0900 on 29 Nov (1354204941), Kouya Shimura wrote:
> > I'm not sure why paging_lock() is used partially in hap_XXX_vram_tracking
> > functions. Thus, this patch introduces a new lock.
> > It would be better to use paging_lock() instead of the new lock
> > since shadow paging mode (not HAP mode) uses paging_lock to avoid
> > this race condition.
> 
> I think you're right - it would be better to use the paging_lock.
> 
> Cc'ing Robert Phillips, who's got a big patch outstanding that touches
> the locking in this code.  I think the right thing to do is make sure
> his patch fixes the issue and the backport just the locking parts of it
> to older trees.
> 
> 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.

-- rsp

> 
> Cheers,
> 
> Tim.

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