On Thu, Mar 14, 2024 at 6:20 PM Christian König
<christian.koenig@xxxxxxx> wrote:
Sending that out once more since the AMD email servers have converted itDoes Christoph's objection come from my poorly worded cover letter and
to HTML mail once more :(
Grrr,
Christian.
Am 14.03.24 um 10:18 schrieb Christian König:
Am 13.03.24 um 18:26 schrieb Sean Christopherson:
On Wed, Mar 13, 2024, Christian König wrote:Because the TTM allocation isn't broken in any way.
Am 13.03.24 um 16:47 schrieb Sean Christopherson:I am so confused. If you agree with Christoph, why not fix the TTM allocations?
[SNIP]And I can only repeat myself that I completely agree with Christoph here.
It can trivially be that userspace only maps 4KiB of some 2MiB piece ofUnless I've wildly misread multiple threads, that is not Christoph's objection.
memory the driver has allocate.
I.e. Christoph is (implicitly) saying that instead of modifying KVM to play nice,Well as far as I can see Christoph rejects the complexity coming with the
we should instead fix the TTM allocations. And David pointed out that that was
tried and got NAK'd.
approach of sometimes grabbing the reference and sometimes not.
From v9 (https://lore.kernel.org/all/ZRpiXsm7X6BFAU%2Fy@xxxxxxxxxxxxx):
On Sun, Oct 1, 2023 at 11:25 PM Christoph Hellwig<hch@xxxxxxxxxxxxx> 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 valid but
> > non-reference-counted pages"). I don't think it makes any sense whatsoever to
> > remove that code and assume every driver in existence will do the right thing.
>
> Agreed.
>
> >
> > With the cleanups done, playing nice with non-refcounted paged instead of outright
> > rejecting them is a wash in terms of lines of code, complexity, and ongoing
> > maintenance cost.
>
> I tend to strongly disagree with that, though. We can't just let these
> non-refcounted pages spread everywhere and instead need to fix their
> usage.
See in some configurations TTM even uses the DMA API for those
allocations and that is actually something Christoph coded.
What Christoph is really pointing out is that absolutely nobody should
put non-refcounted pages into a VMA, but again this isn't something
TTM does. What TTM does instead is to work with the PFN and puts that
into a VMA.
It's just that then KVM comes along and converts the PFN back into a
struct page again and that is essentially what causes all the
problems, including CVE-2021-22543.
commit messages, then?
Fundamentally, what this series is doing is
allowing pfns returned by follow_pte to be mapped into KVM's shadow
MMU without inadvertently translating them into struct pages.
If I'm understand this discussion correctly, since KVM's shadow MMU is hooked
up to MMU notifiers, this shouldn't be controversial. However, my
cover letter got a little confused because KVM is currently doing
something that it sounds like it shouldn't - translating pfns returned
by follow_pte into struct pages with kvm_try_get_pfn. Because of that,
the specific type of pfns that don't work right now are pfn_valid() &&
!PG_Reserved && !page_ref_count() - what I called the non-refcounted
pages in a bad choice of words. If that's correct, then perhaps this
series should go a little bit further in modifying
hva_to_pfn_remapped, but it isn't fundamentally wrong.
-David