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]
Message-ID: <165f07deb8d1e082756e6cae21a25b0060c18f85.camel@infradead.org>
Date: Tue, 06 Feb 2024 19:29:45 -0800
From: David Woodhouse <dwmw2@...radead.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm <kvm@...r.kernel.org>, Paul Durrant <paul@....org>, Paolo Bonzini
 <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
 <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
 <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin"
 <hpa@...or.com>,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery

On Tue, 2024-02-06 at 18:58 -0800, Sean Christopherson wrote:
> On Tue, Feb 06, 2024, David Woodhouse wrote:
> > On Tue, 2024-02-06 at 10:41 -0800, Sean Christopherson wrote:
> > > 
> > > This has an obvious-in-hindsight recursive deadlock bug.  If KVM actually needs
> > > to inject a timer IRQ, and the fast path fails, i.e. the gpc is invalid,
> > > kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is already held
> > 
> > Hm, right. In fact, kvm_xen_set_evtchn() shouldn't actually *need* the
> > xen_lock in an ideal world; it's only taking it in order to work around
> > the fact that the gfn_to_pfn_cache doesn't have its *own* self-
> > sufficient locking. I have patches for that...
> > 
> > I think the *simplest* of the "patches for that" approaches is just to
> > use the gpc->refresh_lock to cover all activate, refresh and deactivate
> > calls. I was waiting for Paul's series to land before sending that one,
> > but I'll work on it today, and double-check my belief that we can then
> > just drop xen_lock from kvm_xen_set_evtchn().
> 
> While I definitely want to get rid of arch.xen.xen_lock, I don't want to address
> the deadlock by relying on adding more locking to the gpc code.  I want a teeny
> tiny patch that is easy to review and backport.  Y'all are *proably* the only
> folks that care about Xen emulation, but even so, that's not a valid reason for
> taking a roundabout way to fixing a deadlock.

I strongly disagree. I get that you're reticent about fixing the gpc
locking, but what I'm proposing is absolutely *not* a 'roundabout way
to fixing a deadlock'. The kvm_xen_set_evtchn() function shouldn't
*need* that lock; it's only taking it because of the underlying problem
with the gpc itself, which needs its caller to do its locking for it.

The solution is not to do further gymnastics with the xen_lock.

> Can't we simply not take xen_lock in kvm_xen_vcpu_get_attr()  It holds vcpu->mutex
> so it's mutually exclusive with kvm_xen_vcpu_set_attr(), and I don't see any other
> flows other than vCPU destruction that deactivate (or change) the gpc.

Maybe. Although with the gpc locking being incomplete, I'm extremely
concerned about something *implicitly* relying on the xen_lock. We
still need to fix the gpc to have self-contained locking.

I'll put something together and do some testing.


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ