From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH] vm_event: make sure the domain is paused in key domctls Date: Fri, 29 Jan 2016 08:52:45 +0200 Message-ID: <56AB0C3D.1010001@bitdefender.com> References: <1453989160-8002-1-git-send-email-rcojocaru@bitdefender.com> <56AA8281.8020001@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Andrew Cooper , Keir Fraser , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org On 01/28/2016 11:47 PM, Tamas K Lengyel wrote: > > > On Thu, Jan 28, 2016 at 2:05 PM, Razvan Cojocaru > > wrote: > > On 01/28/2016 10:09 PM, Tamas K Lengyel wrote: > > On Thu, Jan 28, 2016 at 6:52 AM, Razvan Cojocaru > > > >> wrote: > > > > This patch pauses the domain for all writes through the 'ad' > > pointer in monitor_domctl(), defers a domain_unpause() call until > > after the CRs are updated for the MONITOR_EVENT_WRITE_CTRLREG > > case, and makes sure that the domain is paused for both vm_event > > enable and disable cases in vm_event_domctl(). > > Thanks go to Andrew Cooper for his review and suggestions. > > > > > > For vm_event_enable the domain is already paused by libxc before the > > domctl is issued. I don't see a problem in doing another pause in Xen, > > but given we have XSA-99, just doing this pause in Xen would not be > > enough. So is it really necessary/fixes anything? > > This isn't about XSA-99, the problem here is related to my previous > patch "x86 vm_event: reset monitor in vm_event_cleanup_domain()". While > that improves matters and greatly reduces the chances of crashes due to > hvm_msr_write_intercept() or hvm_set_crX() dereferencing a NULL > v->arch.vm_event that's assumed to be OK, when the corresponding > v->domain->arch.monitor is non-zero, the foolproof way is to make sure > that functions such as vm_event_cleanup_domain() are always being called > only while the domain has been paused. So there should be a > domain_pause() call somewhere on the call path before that. > > > Sure, but that's the disable case. I was only wondering about the enable > case where the domain is already paused. Yes, for the disable case it is functionally redundant. I just thought it would be consistent to use domain_pause() throughout and didn't think it would hurt anything. But I have nothing against removing the domain_pause() for the enable case (perhaps replacing it with a comment that libxc already pauses externally), unless Andrew has any objections. Thanks, Razvan