linux-kernel - Re: [PATCH 2/5 V5] Add functions to check if the host has stopped the vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 14 Dec 2011 12:11:05 -0500
From:	Eric B Munson <emunson@...bm.net>
To:	Avi Kivity <avi@...hat.com>
Cc:	Marcelo Tosatti <mtosatti@...hat.com>, mingo@...hat.com,
	hpa@...or.com, arnd@...db.de, ryanh@...ux.vnet.ibm.com,
	aliguori@...ibm.com, jeremy.fitzhardinge@...rix.com,
	levinsasha928@...il.com, Jan Kiszka <jan.kiszka@...mens.com>,
	kvm@...r.kernel.org, linux-arch@...r.kernel.org, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5 V5] Add functions to check if the host has stopped
 the vm

On Wed, 14 Dec 2011, Avi Kivity wrote:

> On 12/14/2011 02:11 PM, Marcelo Tosatti wrote:
> > On Thu, Dec 08, 2011 at 10:23:10AM -0500, Eric B Munson wrote:
> > > On Wed, 07 Dec 2011, Avi Kivity wrote:
> > > 
> > > > On 12/05/2011 10:19 PM, Eric B Munson wrote:
> > > > > When a host stops or suspends a VM it will set a flag to show this.  The
> > > > > watchdog will use these functions to determine if a softlockup is real, or the
> > > > > result of a suspended VM.
> > > > >  
> > > > > +bool kvm_check_and_clear_guest_paused(int cpu)
> > > > > +{
> > > > > +	bool ret = false;
> > > > > +	struct pvclock_vcpu_time_info *src;
> > > > > +
> > > > > +	/*
> > > > > +	 * per_cpu() is safe here because this function is only called from
> > > > > +	 * timer functions where preemption is already disabled.
> > > > > +	 */
> > > > > +	WARN_ON(!in_atomic());
> > > > > +	src = &per_cpu(hv_clock, cpu);
> > > > 
> > > > __get_cpu_var(); drop the cpu argument
> > > > 
> > > 
> > > Will change for V6.
> > > 
> > > > > +	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
> > > > > +		src->flags = src->flags & (~PVCLOCK_GUEST_STOPPED);
> > > > 
> > > > Isn't this racy?  Between reading and writing src->flags, we can exit to
> > > > the hypervisor and add/remove new flags.  The write then overrides those
> > > > new flags.
> > > > 
> > > 
> > > If I understand (please correct me if this is wrong) because this is only
> > > called from the watchdog, which disables preemption, we should be protected
> > > from something else writing to these flags.
> >
> > The host can write, but in that case race is harmless.
> 
> Why is it harmless?  You don't know what's in those other flags.
> 
> -- 
> error compiling committee.c: too many arguments to function
> 

Currently there is only one other flag in this byte (PVCLOCK_TSC_STABLE_BIT)
and it isset once in kvmclock_init().  It is highly unlikely that the vm will
be stopped during this init and have the flag clobbered.  After the tsc stable
bit is written in the init, the field is read only outside of the guest paused
code.

Eric

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)