On Fri, Feb 16, 2024, Paul Durrant wrote: > On 16/02/2024 13:04, David Woodhouse wrote: > > On Thu, 2024-02-15 at 15:29 +0000, Paul Durrant wrote: > > > From: David Woodhouse <dwmw@xxxxxxxxxxxx> > > > > > > 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. Heh, I'm not taking that bet. > > 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. FWIW, I'm not opposed to overhauling the gpc locking, I agree it's a mess. I just want to proceed slower than I would for a fix, it's a lot to digest. > Ok. Sean, I assume that since this is the last patch in the series it's > superfluous for me to post a v14 just for this? Correct, definitely no need for a new version.
