linux-kernel - Re: [EXTERNAL] [PATCH v2] KVM: Don't actually set a request when evicting vCPUs for GFN cache invd

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, 25 Feb 2022 20:15:45 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     "borntraeger@...ux.ibm.com" <borntraeger@...ux.ibm.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "frankja@...ux.ibm.com" <frankja@...ux.ibm.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "imbrenda@...ux.ibm.com" <imbrenda@...ux.ibm.com>,
        "david@...hat.com" <david@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [EXTERNAL] [PATCH v2] KVM: Don't actually set a request when
 evicting vCPUs for GFN cache invd

On Fri, 2022-02-25 at 18:52 +0000, Sean Christopherson wrote:
> On Fri, Feb 25, 2022, David Woodhouse wrote:
> > > > I think I'm going to rip out the 'dirty' flag from the gfn_to_pfn_cache
> > > > completely and add a function (to be called with an active vCPU
> > > > context) which marks the page dirty *now*.
> > > 
> > > Hrm, something like?
> > > 
> > >   1. Drop @dirty from kvm_gfn_to_pfn_cache_init()
> > >   2. Rename @dirty => @old_dirty in kvm_gfn_to_pfn_cache_refresh()
> > >   3. Add an API to mark the associated slot dirty without unmapping
> > > 
> > > I think that makes sense.
> > 
> > Except I'll drop 'dirty' from kvm_gfn_to_pfn_cache_refresh() too.
> > There's no scope for a deferred "oh, I meant to tell you that was
> > dirty" even in that case, is there? Use the API we add in your #3.
> 
> But won't we end up with a bunch of call sites that attempt to determine whether
> not the dirty status needs to be flushed?  I'm specifically thinking of scenarios
> where the status needs to be conditionally flushed, e.g. if the backing pfn doesn't
> change, then it's ok to not mark the page dirty.  Not handling that in the refresh
> helper will either lead to unnecessary dirtying or duplicate code/work.

I don't know that it's ever OK to leave the dirty status cached in the
GPC to be flushed at some unspecified "later" time. Once the vCPU exits
back to userspace, that might be the *last* time it's ever run, and we
have the same ABI issue as (snipped) above, that userspace might have
done its final read of the dirty log, and might not expect anything
else to be marked dirty.

We might be able to defer it until we return to userspace, but no
further. So I don't see flushing it in *refresh* as being useful? And
we *can't* flush it in invalidate anyway, as discussed.


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