Received-SPF: pass (google.com: domain of linux-kernel+bounces-101696-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 13 Mar 2024 15:47:47 +0000
In-Reply-To: <9e604f99-5b63-44d7-8476-00859dae1dc4@amd.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240229025759.1187910-1-stevensd@google.com> <ZeCIX5Aw5s1L0YEh@infradead.org>
 <CAD=HUj7fT2CVXLfi5mty0rSzpG_jK9fhcKYGQnTf_H8Hg-541Q@mail.gmail.com>
 <72285e50-6ffc-4f24-b97b-8c381b1ddf8e@amd.com> <ZfGrS4QS_WhBWiDl@google.com>
 <0b109bc4-ee4c-4f13-996f-b89fbee09c0b@amd.com> <ZfG801lYHRxlhZGT@google.com> <9e604f99-5b63-44d7-8476-00859dae1dc4@amd.com>
Message-ID: <ZfHKoxVMcBAMqcSC@google.com>
Subject: Re: [PATCH v11 0/8] KVM: allow mapping non-refcounted pages
From: Sean Christopherson <seanjc@google.com>
To: "Christian =?utf-8?B?S8O2bmln?=" <christian.koenig@amd.com>
Cc: David Stevens <stevensd@chromium.org>, Christoph Hellwig <hch@infradead.org>, 
	Paolo Bonzini <pbonzini@redhat.com>, Yu Zhang <yu.c.zhang@linux.intel.com>, 
	Isaku Yamahata <isaku.yamahata@gmail.com>, Zhi Wang <zhi.wang.linux@gmail.com>, 
	Maxim Levitsky <mlevitsk@redhat.com>, kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, 
	kvm@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 13, 2024, Christian K=C3=B6nig wrote:
> Am 13.03.24 um 15:48 schrieb Sean Christopherson:
> > On Wed, Mar 13, 2024, Christian K=C3=B6nig wrote:
> > > Am 13.03.24 um 14:34 schrieb Sean Christopherson:
> > > > What Christoph is objecting to is that, in this series, KVM is expl=
icitly adding
> > > > support for mapping non-compound (huge)pages into KVM guests.  Davi=
d is arguing
> > > > that Christoph's objection to _KVM_ adding support is unfair, becau=
se the real
> > > > problem is that the kernel already maps such pages into host usersp=
ace.  I.e. if
> > > > the userspace mapping ceases to exist, then there are no mappings f=
or KVM to follow
> > > > and propagate to KVM's stage-2 page tables.
> > > And I have to agree with Christoph that this doesn't make much sense.=
 KVM
> > > should *never* map (huge) pages from VMAs marked with VM_PFNMAP into =
KVM
> > > guests in the first place.
> > >=20
> > > What it should do instead is to mirror the PFN from the host page tab=
les
> > > into the guest page tables.
> > That's exactly what this series does.  Christoph is objecting to KVM pl=
aying nice
> > with non-compound hugepages, as he feels that such mappings should not =
exist
> > *anywhere*.
>=20
> Well Christoph is right those mappings shouldn't exists and they also don=
't
> exists.
>
> What happens here is that a driver has allocated some contiguous memory t=
o
> do DMA with. And then some page table is pointing to a PFN inside that
> memory because userspace needs to provide parameters for the DMA transfer=
.
>=20
> This is *not* a mapping of a non-compound hugepage, it's simply a PTE
> pointing to some PFN.=20

Yes, I know.  And David knows.  By "such mappings" I did not mean "huge PMD=
 mappings
that point at non-compound pages", I meant "any mapping in the host userspa=
ce
VMAs and page tables that points at memory that is backed by a larger-than-=
order-0,
non-compound allocation".

And even then, the whole larger-than-order-0 mapping is not something we on=
 the
KVM side care about, at all.  The _only_ new thing KVM is trying to do in t=
his
series is to allow mapping non-refcounted struct page memory into KVM guest=
.
Those details were brought up purely because they provide context on how/wh=
y such
non-refcounted pages exist.

> It can trivially be that userspace only maps 4KiB of some 2MiB piece of
> memory the driver has allocate.
>
> > I.e. Christoph is (implicitly) saying that instead of modifying KVM to =
play nice,
> > we should instead fix the TTM allocations.  And David pointed out that =
that was
> > tried and got NAK'd.
>=20
> Well as far as I can see Christoph rejects the complexity coming with the
> approach of sometimes grabbing the reference and sometimes not.

Unless I've wildly misread multiple threads, that is not Christoph's object=
ion.
From v9 (https://lore.kernel.org/all/ZRpiXsm7X6BFAU%2Fy@infradead.org):

  On Sun, Oct 1, 2023 at 11:25=E2=80=AFPM Christoph Hellwig <hch@infradead.=
org> wrote:
  >
  > On Fri, Sep 29, 2023 at 09:06:34AM -0700, Sean Christopherson wrote:
  > > KVM needs to be aware of non-refcounted struct page memory no matter =
what; see
  > > CVE-2021-22543 and, commit f8be156be163 ("KVM: do not allow mapping v=
alid but
  > > non-reference-counted pages"). =C2=A0I don't think it makes any sense=
 whatsoever to
  > > remove that code and assume every driver in existence will do the rig=
ht thing.
  >
  > Agreed.
  >
  > >
  > > With the cleanups done, playing nice with non-refcounted paged instea=
d of outright
  > > rejecting them is a wash in terms of lines of code, complexity, and o=
ngoing
  > > maintenance cost.
  >
  > I tend to strongly disagree with that, though. =C2=A0We can't just let =
these
  > non-refcounted pages spread everywhere and instead need to fix their
  > usage.

> And I have to agree that this is extremely odd.

Yes, it's odd and not ideal.  But with nested virtualization, KVM _must_ "m=
ap"
pfns directly into the guest via fields in the control structures that are
consumed by hardware.  I.e. pfns are exposed to the guest in an "out-of-ban=
d"
structure that is NOT part of the stage-2 page tables.  And wiring those up=
 to
the MMU notifiers is extremely difficult for a variety of reasons[*].

Because KVM doesn't control which pfns are mapped this way, KVM's compromis=
e is
to grab a reference to the struct page while the out-of-band mapping exists=
, i.e.
to pin the page to prevent use-after-free.  And KVM's historical ABI is to =
support
any refcounted page for these out-of-band mappings, regardless of whether t=
he
page was obtained by gup() or follow_pte().

Thus, to support non-refouncted VM_PFNMAP pages without breaking existing u=
serspace,
KVM resorts to conditionally grabbing references and disllowing non-refcoun=
ted
pages from being inserted into the out-of-band mappings.

But again, I don't think these details are relevant to Christoph's objectio=
n.

[*] https://lore.kernel.org/all/ZBEEQtmtNPaEqU1i@google.com