Re: [PATCH v13 21/21] KVM: pfncache: rework __kvm_gpc_refresh() to fix locking issues

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On Thu, 2024-02-15 at 15:29 +0000, Paul Durrant wrote:
> From: David Woodhouse <dwmw@amazon.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.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]