linux-kernel - Re: [PATCH v3 3/8] KVM: x86/mmu: Rename NX huge pages fields/functions for consistency

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yv0Tk0WAdxymSyUt@google.com>
Date:   Wed, 17 Aug 2022 16:13:07 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Mingwei Zhang <mizhang@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, David Matlack <dmatlack@...gle.com>,
        Yan Zhao <yan.y.zhao@...el.com>,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [PATCH v3 3/8] KVM: x86/mmu: Rename NX huge pages
 fields/functions for consistency

On Tue, Aug 16, 2022, Mingwei Zhang wrote:
> On Mon, Aug 15, 2022, Sean Christopherson wrote:
> > On Sun, Aug 14, 2022, Mingwei Zhang wrote:
> > > On Fri, Aug 05, 2022, Sean Christopherson wrote:
> > > > Rename most of the variables/functions involved in the NX huge page
> > > > mitigation to provide consistency, e.g. lpage vs huge page, and NX huge
> > > > vs huge NX, and also to provide clarity, e.g. to make it obvious the flag
> > > > applies only to the NX huge page mitigation, not to any condition that
> > > > prevents creating a huge page.
> > > > 
> > > > Leave the nx_lpage_splits stat alone as the name is ABI and thus set in
> > > > stone.
> > > > 
> > > > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > > > ---
> > > >  arch/x86/include/asm/kvm_host.h |  8 ++--
> > > >  arch/x86/kvm/mmu/mmu.c          | 70 +++++++++++++++++----------------
> > > >  arch/x86/kvm/mmu/mmu_internal.h | 22 +++++++----
> > > >  arch/x86/kvm/mmu/paging_tmpl.h  |  2 +-
> > > >  arch/x86/kvm/mmu/tdp_mmu.c      |  8 ++--
> > > >  5 files changed, 59 insertions(+), 51 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > index e8281d64a431..5634347e5d05 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -1143,7 +1143,7 @@ struct kvm_arch {
> > > >  	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> > > >  	struct list_head active_mmu_pages;
> > > >  	struct list_head zapped_obsolete_pages;
> > > > -	struct list_head lpage_disallowed_mmu_pages;
> > > > +	struct list_head possible_nx_huge_pages;
> > > 
> > > Honestly, I am struggling to understand this one. 'possible_*' indicates
> > > that there are other possibilities. But what are those possibilities?
> > 
> > No, possible is being used as an adjective in this case.  possible_nx_huge_pages
> > is the list of shadow pages for which it's possible to replace the shadow page
> > with an NX huge page.
> > 
> > The noun version would yield a name like nx_huge_page_possiblities.
> 
> Right, but these shadow pages are not NX huge pages, right? IIUC, they
> are pages to be zapped due to NX huge pages, aren't they?

Yes, they are shadow pages that the NX recovery thread should zap, but the reason
they should be zapped is because (a) the shadow page has at least one execute child
SPTE, (b) zapping the shadow page will also zap its child SPTEs, and (c) eliminating
all executable child SPTEs means KVM _might_ be able to instantiate an NX huge page.

> `nx_huge_page_disallowed` is easy to understand because it literally say
> 'nx_huge_page is not allowed', which is correct.

No, it's not correct.  The list isn't simply the set of shadow pages that disallow
NX huge pages, it's the set of shadow pages that disallow NX huge pages _and_ that
can possibly be replaced by an NX huge page if the shadow page and all its
(executable) children go away.

> But this one, it says 'possible nx_huge_pages', but they are not
> nx huge pages at all.

Yes, but they _can be_ NX huge pages, hence the "possible".  A super verbose name
would be something like mmu_pages_that_can_possibly_be_replaced_by_nx_huge_pages.

> Instead, they are 'shadow pages that are replaced with nx_huge_pages'. So
> that's why updates to this list is done together with stats nx_plage_splits.
> 
> Please correct me if I am wrong. I am still struggling to understand the
> meaning of these variables.
> 
> >
> > > I feel this name is more confusing than the original one. Maybe just keep
> > 
> > Ignoring lpage => huge_page, the current name is terribly inaccurate.  The list
> > doesn't contain all disallowed huge pages, nor does it even contain all disallowed
> > NX huge pages, it specifically tracks shadow pages that might be able to be
> > replaced with an NX huge page.
> > 
> > I'm open to other names, but whatever we choose should be paired with
> > account_nx_huge_page()'s param that is currently named "nx_huge_page_possible".
> 
> How about mmu_pages_replaced_by_nx_huge,

"replaced" is past tense in this usage, and these are definitely not shadow pages
that have already been replaced by NX huge pages.

> mmu_pages_replaced_by_possible_nx_huge or something starting with

Same issue with "replaced".

> possible_pages_, pages_ instead of possible_nx_huge_pages?

Reprhasing, I think you're asking that the variable have "mmu_pages" somewhere in
the name to clarify that it's a list of kvm_mmu_page structs.  I agree that ideally
it would have "mmu_pages" somewhere in there, but I also think that having both
"nx_huge_pages" and "mmu_pages" in the name makes it unreadable, i.e. we have to
choose one use of "pages".

The only alternative I can come up with that isn't completely gross is
possible_nx_huge_mmu_pages.  I can't tell if that's less confusing or more
confusing.