linux-kernel - Re: [PATCH 19/21] KVM: TDX: Add an ioctl to create initial guest memory

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: <9983d4229ad0f6c75605da8846253d1ffca84ae8.camel@intel.com>
Date: Fri, 6 Sep 2024 16:30:00 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Zhao, Yan Y" <yan.y.zhao@...el.com>
CC: "seanjc@...gle.com" <seanjc@...gle.com>, "Huang, Kai"
	<kai.huang@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "dmatlack@...gle.com" <dmatlack@...gle.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "nik.borisov@...e.com"
	<nik.borisov@...e.com>, "pbonzini@...hat.com" <pbonzini@...hat.com>
Subject: Re: [PATCH 19/21] KVM: TDX: Add an ioctl to create initial guest
 memory

On Wed, 2024-09-04 at 07:01 -0700, Rick Edgecombe wrote:
> On Wed, 2024-09-04 at 12:53 +0800, Yan Zhao wrote:
> > > +       if (!kvm_mem_is_private(kvm, gfn)) {
> > > +               ret = -EFAULT;
> > > +               goto out_put_page;
> > > +       }
> > > +
> > > +       ret = kvm_tdp_map_page(vcpu, gpa, error_code, &level);
> > > +       if (ret < 0)
> > > +               goto out_put_page;
> > > +
> > > +       read_lock(&kvm->mmu_lock);
> > Although mirrored root can't be zapped with shared lock currently, is it
> > better to hold write_lock() here?
> > 
> > It should bring no extra overhead in a normal condition when the
> > tdx_gmem_post_populate() is called.
> 
> I think we should hold the weakest lock we can. Otherwise someday someone
> could
> run into it and think the write_lock() is required. It will add confusion.
> 
> What was the benefit of a write lock? Just in case we got it wrong?

I just tried to draft a comment to make it look less weird, but I think actually
even the mmu_read lock is technically unnecessary because we hold both
filemap_invalidate_lock() and slots_lock. The cases we care about:
 memslot deletion - slots_lock protects
 gmem hole punch - filemap_invalidate_lock() protects
 set attributes - slots_lock protects
 others?

So I guess all the mirror zapping cases that could execute concurrently are
already covered by other locks. If we skipped grabbing the mmu lock completely
it would trigger the assertion in kvm_tdp_mmu_gpa_is_mapped(). Removing the
assert would probably make kvm_tdp_mmu_gpa_is_mapped() a bit dangerous. Hmm. 

Maybe a comment like this:
/*
 * The case to care about here is a PTE getting zapped concurrently and 
 * this function erroneously thinking a page is mapped in the mirror EPT.
 * The private mem zapping paths are already covered by other locks held
 * here, but grab an mmu read_lock to not trigger the assert in
 * kvm_tdp_mmu_gpa_is_mapped().
 */

Yan, do you think it is sufficient?