linux-kernel - Re: [PATCH 2/3] KVM: x86/mmu: Take "shared" instead of "as

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]

Message-ID: <ZQxUCc3BEHA91FgY@google.com>
Date:   Thu, 21 Sep 2023 14:32:41 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Pattara Teerapong <pteerapong@...gle.com>,
        David Stevens <stevensd@...gle.com>,
        Yiwei Zhang <zzyiwei@...gle.com>,
        Paul Hsia <paulhsia@...gle.com>
Subject: Re: [PATCH 2/3] KVM: x86/mmu: Take "shared" instead of "as_id" TDP
 MMU's yield-safe iterator

On Thu, Sep 21, 2023, Paolo Bonzini wrote:
> On 9/16/23 02:39, Sean Christopherson wrote:
> > Replace the address space ID in for_each_tdp_mmu_root_yield_safe() with a
> > shared (vs. exclusive) param, and have the walker iterate over all address
> > spaces as all callers want to process all address spaces.  Drop the @as_id
> > param as well as the manual address space iteration in callers.
> > 
> > Add the @shared param even though the two current callers pass "false"
> > unconditionally, as the main reason for refactoring the walker is to
> > simplify using it to zap invalid TDP MMU roots, which is done with
> > mmu_lock held for read.
> > 
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> 
> You konw what, I don't really like the "bool shared" arguments anymore.

Yeah, I don't like the "shared" arguments either.  Never did, but they are necessary
for some paths, and I don't see an obviously better solution. :-/

> For example, neither tdp_mmu_next_root nor kvm_tdp_mmu_put_root need to know
> if the lock is taken for read or write; protection is achieved via RCU and
> tdp_mmu_pages_lock.  It's more self-documenting to remove the argument and
> assert that the lock is taken.
> 
> Likewise, the argument is more or less unnecessary in the
> for_each_*_tdp_mmu_root_yield_safe() macros.  Many users check for the lock
> before calling it; and all of them either call small functions that do the
> check, or end up calling tdp_mmu_set_spte_atomic() and
> tdp_mmu_iter_set_spte(), so the per-iteration checks are also overkill.

Agreed.
 
> It may be useful to a few assertions to make up for the lost check before
> the first execution of the body of for_each_*_tdp_mmu_root_yield_safe(), but
> even this is more for documentation reasons than to catch actual bugs.

I think it's more than sufficient, arguably even better, to document which paths
*require* mmu_lock be held for read vs. write, and which paths work with either.

> I'll send a v2.

Can we do a cleanup of the @shared arguments on top?  I would like to keep the
diff reasonably small to minimize the v6.1 backport.