Re: [PATCH v11 0/8] KVM: allow mapping non-refcounted pages

["Christian König" <christian.koenig@amd.com>]

Am 14.03.24 um 12:31 schrieb David Stevens:
> On Thu, Mar 14, 2024 at 6:20 PM Christian König
> <christian.koenig@amd.com> wrote:
>> Sending that out once more since the AMD email servers have converted it
>> 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:
>>>>> Am 13.03.24 um 16:47 schrieb Sean Christopherson:
>>>>>> [SNIP]
>>>>>>> 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.
>>>>>>> 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 objection.
>>>>>>    From v9 (https://lore.kernel.org/all/ZRpiXsm7X6BFAU%2Fy@infradead.org):
>>>>>>
>>>>>>      On Sun, Oct 1, 2023 at 11:25 PM 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 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.
>>>>> And I can only repeat myself that I completely agree with Christoph here.
>>>> I am so confused.  If you agree with Christoph, why not fix the TTM allocations?
>>> Because the TTM allocation isn't broken in any way.
>>>
>>> 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.
> Does Christoph's objection come from my poorly worded cover letter and
> commit messages, then?

Yes, that could certainly be.

> 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.

As far as I can tell that is really the right thing to do. Yes.

> 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.

Completely agree. In my thinking when you go a step further and offload 
grabbing the page reference to get_user_pages() then you are always on 
the save side.

Because then it is no longer the responsibility of the KVM code to get 
all the rules around that right, instead you are relying on a core 
functionality which should (at least in theory) do the correct thing.

Regards,
Christian.

>
> -David