linux-kernel - Re: [PATCH v13 21/21] KVM: pfncache: rework __kvm_gpc

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: Fri, 16 Feb 2024 13:04:55 +0000
From: David Woodhouse <dwmw2@...radead.org>
To: Paul Durrant <paul@....org>, Paolo Bonzini <pbonzini@...hat.com>, 
 Jonathan Corbet <corbet@....net>, Christian Borntraeger
 <borntraeger@...ux.ibm.com>, Janosch Frank <frankja@...ux.ibm.com>, Claudio
 Imbrenda <imbrenda@...ux.ibm.com>, David Hildenbrand <david@...hat.com>,
 Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>,
 Alexander Gordeev <agordeev@...ux.ibm.com>, Sven Schnelle
 <svens@...ux.ibm.com>, Sean Christopherson <seanjc@...gle.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>, Shuah Khan
 <shuah@...nel.org>, kvm@...r.kernel.org,  linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org,  linux-s390@...r.kernel.org,
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v13 21/21] KVM: pfncache: rework __kvm_gpc_refresh() to
 fix locking issues

On Thu, 2024-02-15 at 15:29 +0000, Paul Durrant wrote:
> From: David Woodhouse <dwmw@...zon.co.uk>
> 
> This function can race with kvm_gpc_deactivate(), which does not take
> the ->refresh_lock. This means kvm_gpc_deactivate() can wipe the ->pfn
> and ->khva fields, and unmap the latter, while hva_to_pfn_retry() has
> temporarily dropped its write lock on gpc->lock.

Let's drop this from your series for now, as it's contentious.

Sean didn't like calling it a 'fix', which I had conceded and reworked
the commit message. It was on the list somewhere, and also in
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/f19755000a7

I *also* think we should do this simpler one:
https://git.infradead.org/users/dwmw2/linux.git/commitdiff/cc69506d19a
.. which almost makes the first one unnecessary, but I think we should
do it *anyway* because the rwlock abuse it fixes is kind of awful.

And while we still can't actually *identify* the race condition that
led to a dereference of a NULL gpc->khva while holding the read lock
and gpc->valid and gpc->active both being true... I'll eat my hat if
cleaning up and simplifying the locking (and making it self-contained)
*doesn't* fix it.

But either way, it isn't really part of your series. The only reason it
was tacked on the end was because it would have merge conflicts with
your series, which had been outstanding for months already.

So drop this one, and I'll work this bit out with Sean afterwards.

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