Date:   Wed, 6 Sep 2023 01:29:57 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Yan Zhao <yan.y.zhao@...el.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 0/2] KVM: x86/mmu: .change_pte() optimization in TDP MMU

On 2023-09-05 19:59, Sean Christopherson wrote:
> +swiotbl maintainers and Linus
> 
> Spinning off a discussion about swiotlb behavior to its own thread.
> 
> Quick background: when running Linux as a KVM guest, Yan observed memory accesses
> where Linux is reading completely uninitialized memory (never been written by the
> guest) and traced it back to this code in the swiotlb:
> 
> 	/*
> 	 * When dir == DMA_FROM_DEVICE we could omit the copy from the orig
> 	 * to the tlb buffer, if we knew for sure the device will
> 	 * overwrite the entire current content. But we don't. Thus
> 	 * unconditional bounce may prevent leaking swiotlb content (i.e.
> 	 * kernel memory) to user-space.
> 	 */
> 	swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE);
> 
> The read-before-write behavior results in suboptimal performance as KVM maps the
> memory as read-only, and then triggers CoW when the guest inevitably writes the
> memory (CoW is significantly more expensive when KVM is involved).
> 
> There are a variety of ways to workaround this in KVM, but even if we decide to
> address this in KVM, the swiotlb behavior is sketchy.  Not to mention that any
> KVM changes are highly unlikely to be backported to LTS kernels.
> 
> On Mon, Sep 04, 2023, Yan Zhao wrote:
>> ...
>>>> Actually, I don't even completely understand how you're seeing CoW behavior in
>>>> the first place.  No sane guest should blindly read (or execute) uninitialized
>>>> memory.  IIUC, you're not running a Windows guest, and even if you are, AFAIK
>>>> QEMU doesn't support Hyper-V's enlightment that lets the guest assume memory has
>>>> been zeroed by the hypervisor.  If KSM is to blame, then my answer it to turn off
>>>> KSM, because turning on KSM is antithetical to guest performance (not to mention
>>>> that KSM is wildly insecure for the guest, especially given the number of speculative
>>>> execution attacks these days).
>>> I'm running a linux guest.
>>> KSM is not turned on both in guest and host.
>>> Both guest and host have turned on transparent huge page.
>>>
>>> The guest first reads a GFN in a writable memslot (which is for "pc.ram"),
>>> which will cause
>>>      (1) KVM first sends a GUP without FOLL_WRITE, leaving a huge_zero_pfn or a zero-pfn
>>>          mapped.
>>>      (2) KVM calls get_user_page_fast_only() with FOLL_WRITE as the memslot is writable,
>>>          which will fail
>>>
>>> The guest then writes the GFN.
>>> This step will trigger (huge pmd split for huge page case) and .change_pte().
>>>
>>> My guest is surely a sane guest. But currently I can't find out why
>>> certain pages are read before write.
>>> Will return back to you the reason after figuring it out after my long vacation.
>> Finally I figured out the reason.
>>
>> Except 4 pages were read before written from vBIOS (I just want to skip finding
>> out why vBIOS does this), the remaining thousands of pages were read before
>> written from the guest Linux kernel.
> 
> ...
> 
>> When the guest kernel triggers a guest block device read ahead, pages are
>> allocated as page cache pages, and requests to read disk data into the page
>> cache are issued.
>>
>> The disk data read requests will cause dma_direct_map_page() called if vIOMMU
>> is not enabled. Then, because the virtual IDE device can only direct access
>> 32-bit DMA address (equal to GPA) at maximum, swiotlb will be used as DMA
>> bounce if page cache pages are with GPA > 32 bits.
>>
>> Then the call path is
>> dma_direct_map_page() --> swiotlb_map() -->swiotlb_tbl_map_single()
>>
>> In swiotlb_tbl_map_single(), though DMA direction is DMA_FROM_DEVICE,
>> this swiotlb_tbl_map_single() will call
>> swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE) to read page cache
>> content to the bounce buffer first.
>> Then, during device DMAs, device content is DMAed into the bounce buffer.
>> After that, the bounce buffer data will be copied back to the page cache page
>> after calling dma_direct_unmap_page() --> swiotlb_tbl_unmap_single().
>>
>>
>> IOW, before reading ahead device data into the page cache, the page cache is
>> read and copied to the bounce buffer, though an immediate write is followed to
>> copy bounce buffer data back to the page cache.
>>
>> This explains why it's observed in host that most pages are written immediately
>> after it's read, and .change_pte() occurs heavily during guest boot-up and
>> nested guest boot-up, -- when disk readahead happens abundantly.
>>
>> The reason for this unconditional read of page into bounce buffer
>> (caused by "swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE)")
>> is explained in the code:
>>
>> /*
>>   * When dir == DMA_FROM_DEVICE we could omit the copy from the orig
>>   * to the tlb buffer, if we knew for sure the device will
>>   * overwrite the entire current content. But we don't. Thus
>>   * unconditional bounce may prevent leaking swiotlb content (i.e.
>>   * kernel memory) to user-space.
>>   */
>>
>> If we neglect this risk and do changes like
>> -       swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE);
>> +       if (dir != DMA_FROM_DEVICE)
>> +               swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE);
>>
>> the issue of pages read before written from guest kernel just went away.
>>
>> I don't think it's a swiotlb bug, because to prevent leaking swiotlb
>> content, if target page content is not copied firstly to the swiotlb's
>> bounce buffer, then the bounce buffer needs to be initialized to 0.
>> However, swiotlb_tbl_map_single() does not know whether the target page
>> is initialized or not. Then, it would cause page content to be trimmed
>> if device does not overwrite the entire memory.
> 
> The math doesn't add up though.  Observing a read-before-write means the intended
> goal of preventing data leaks to userspace is not being met.  I.e. instead of
> leaking old swiotlb, the kernel is (theoretically) leaking whatever data is in the
> original page (page cache in your case).

Sure, it copies the destination page into the SWIOTLB buffer on map, 
then copies it back out again on unmap, so it's only "leaking" the 
previous contents of the page into the same page. Think of it as SWIOTLB 
doing a read-modify-write of the DMA-mapped region, because that's 
exactly what it's doing (on the basis that it can't be sure of 
overwriting it entirely).

You can then consider it the same as if the device DMAs to the page 
directly without SWIOTLB being involved - if it does a partial write and 
that lets previous contents be mapped to userspace which shouldn't have 
been, that can only be the fault of whoever failed to sanitise the page 
beforehand.

> That data *may* be completely uninitialized, especially during boot, but the
> original pages may also contain data from whatever was using the pages before they
> were allocated by the driver.
> 
> It's possible the read of uninitialized data is observed only when either the
> driver that triggered the mapping _knows_ that the device will overwrite the entire
> mapping, or the driver will consume only the written parts.  But in those cases,
> copying from the original memory is completely pointless.

Indeed, but sadly it is impossible for SWIOTLB to detect when that is or 
isn't the case. No matter what the SWIOTLB buffer is initialised with, 
there is always the possibility that the device coincidentally writes 
the same pattern of bytes, thus it cannot ever know for sure what was or 
wasn't touched between map and unmap.

This is why in the original discussion we also tossed around the idea of 
a DMA attribute for the caller to indicate "I definitely expect the 
device to overwrite this entire mapping", but then we went round in 
circles a bit more, concluded that it might be hard to get right, and 
shelved it, but then the wrong version of the patch got merged which did 
still include the half-formed attribute idea, and then there was the 
whole other trainwreck of reverting and unreverting, and I have a 
feeling that what we ended up with still wasn't quite right but by that 
point I was so fed up of the whole business I had stopped caring.

> If neither of the above holds true, then copying from the original adds value only
> if preserving the data is necessary for functional correctness, or the driver
> explicitly initialized the original memory.  Given that preserving source data was
> recently added, I highly doubt it's necessary for functional correctness.

Seems kinda hard to say that with certainty - I would imagine that the 
amount of actual testing done with "swiotlb=force" is minimal at best. 
There are so many 64-bit-capable devices in the world which would never 
normally interact with SWIOTLB, but are liable to find themselves forced 
into doing so in future confidential compute etc. scenarios. I don't 
think I'd bet anything of value that *nothing* anywhere is depending on 
partial DMA writes being handled correctly.

> And if the driver *doesn't* initialize the data, then the copy is at best pointless,
> and possibly even worse than leaking stale swiotlb data.

Other than the overhead, done right it can't be any worse than if 
SWIOTLB were not involved at all.

> Looking at commit ddbd89deb7d3 ("swiotlb: fix info leak with DMA_FROM_DEVICE"),
> IIUC the data leak was observed with a synthetic test "driver" that was developed
> to verify a real data leak fixed by commit a45b599ad808 ("scsi: sg: allocate with
> __GFP_ZERO in sg_build_indirect()").  Which basically proves my point: copying
> from the source only adds value absent a bug in the owning driver.

Huh? IIUC the bug there was that the SCSI layer failed to sanitise 
partially-written buffers. That bug was fixed, and the scrutiny therein 
happened to reveal that SWIOTLB *also* had a lower-level problem with 
partial writes, in that it was corrupting DMA-mapped memory which was 
not updated by the device. Partial DMA writes are not in themselves 
indicative of a bug, they may well be a valid and expected behaviour.

> IMO, rather than copying from the original memory, swiotlb_tbl_map_single() should
> simply zero the original page(s) when establishing the mapping.  That would harden
> all usage of swiotlb and avoid the read-before-write behavior that is problematic
> for KVM.

Depends on one's exact definition of "harden"... Corrupting memory with 
zeros is less bad than corrupting memory with someone else's data if you 
look at it from an information security point of view, but from a 
not-corrupting-memory point of view it's definitely still corrupting 
memory :/

Taking a step back, is there not an argument that if people care about 
general KVM performance then they should maybe stop emulating obsolete 
PC hardware from 30 years ago, and at least emulate obsolete PC hardware 
from 20 years ago that supports 64-bit DMA? Even non-virtualised, 
SWIOTLB is pretty horrible for I/O performance by its very nature - 
avoiding it if at all possible should always be preferred.

Thanks,
Robin.

Powered by blists - more mailing lists