On Tue, 14 Apr 2020 11:50:16 -0700 Dave Hansen <dave.hansen@xxxxxxxxx> wrote: > On 4/14/20 9:03 AM, Claudio Imbrenda wrote: > > On Mon, 13 Apr 2020 13:22:24 -0700 > > Dave Hansen <dave.hansen@xxxxxxxxx> wrote: > > > >> On 3/6/20 5:25 AM, Claudio Imbrenda wrote: > >>> On s390x the function is not supposed to fail, so it is ok to use > >>> a WARN_ON on failure. If we ever need some more finegrained > >>> handling we can tackle this when we know the details. > >> > >> Could you explain a bit why the function can't fail? > > > > the concept of "making accessible" is only to make sure that > > accessing the page will not trigger faults or I/O or DMA errors. in > > general it does not mean freely accessing the content of the page > > in cleartext. > > > > on s390x, protected guest pages can be shared. the guest has to > > actively share its pages, and in that case those pages are both > > part of the protected VM and freely accessible by the host. > > Oh, that's interesting. > > It sounds like there are three separate concepts: > 1. Protection > 2. Sharing > 3. Accessibility > > Protected pages may be shared and the request of the guest. > Shared pages' plaintext can be accessed by the host. For unshared > pages, the host can only see ciphertext. > > I wonder if Documentation/virt/kvm/s390-pv.rst can be beefed up with > some of this information. It seems a bit sparse on this topic. that is definitely something that can be fixed. I will improve the documentation and make sure it properly explains all the details of how protected VMs work on s390x. > As it stands, if I were modifying generic code, I don't think I'd have > even a chance of getting an arch_make_page_accessible() in the right > spot. > > > in our case "making the page accessible" means: > ... > > - if the page was not shared, first encrypt it and then make it > > accessible to the host (both operations performed securely and > > atomically by the hardware) > > What happens to the guest's view of the page when this happens? Does > it keep seeing plaintext? > > > then the page can be swapped out, or used for direct I/O (obviously > > if you do I/O on a page that was not shared, you cannot expect good > > things to happen, since you basically corrupt the memory of the > > guest). > > So why even allow access to the encrypted contents if the host can't > do anything useful with it? Is there some reason for going to the > trouble of encrypting it and exposing it to the host? you should not overwrite it, but you can/should write it out verbatim, e.g. for swap > > on s390x performing I/O directly on protected pages results in (in > > practice) unrecoverable I/O errors, so we want to avoid it at all > > costs. > > This is understandable, but we usually steer I/O operations in places > like the DMA API, not in the core VM. > > We *have* the concept of pages to which I/O can't be done. There are > plenty of crippled devices where we have to bounce data into a low > buffer before it can go where we really want it to. I think the AMD > SEV patches do this, for instance. > > > accessing protected pages from the CPU triggers an exception that > > can be handled (and we do handle it, in fact) > > > > now imagine a buggy or malicious qemu process crashing the whole > > machine just because it did I/O to/from a protected page. we > > clearly don't want that. > > Is DMA disallowed to *all* protected pages? Even pages which the > guest has explicitly shared with the host? > > > >>> @@ -2807,6 +2807,13 @@ int __test_set_page_writeback(struct page > >>> *page, bool keep_write) inc_zone_page_state(page, > >>> NR_ZONE_WRITE_PENDING); } > >>> unlock_page_memcg(page); > >>> + access_ret = arch_make_page_accessible(page); > >>> + /* > >>> + * If writeback has been triggered on a page that cannot > >>> be made > >>> + * accessible, it is too late to recover here. > >>> + */ > >>> + VM_BUG_ON_PAGE(access_ret != 0, page); > >>> + > >>> return ret; > >>> > >>> } > >> > >> This seems like a really odd place to do this. Writeback is > >> specific to block I/O. I would have thought there were other > >> kinds of devices that matter, not just block devices. > > > > well, yes and no. for writeback (block I/O and swap) this is the > > right place. at this point we know that the page is present and > > nobody else has started doing I/O yet, and I/O will happen > > soon-ish. so we make the page accessible. there is no turning back > > here, unlike pinning. we are not allowed to fail, we can't > > This description sounds really incomplete to me. > > Not all swap involved device I/O. For instance, zswap doesn't involve > any devices. Would zswap need this hook? please feel free to write to me privately if you have any further questions or doubts :) best regards, Claudio Imbrenda
