On Wed, Mar 29, 2023, Christian K�nig wrote: > Am 29.03.23 um 19:39 schrieb Sean Christopherson: > > On Wed, Mar 29, 2023, Christian K�nig wrote: > > > Am 29.03.23 um 19:22 schrieb Sean Christopherson: > > > > +David > > > > > > > > On Wed, Mar 29, 2023, Paolo Bonzini wrote: > > > > > On 3/29/23 18:43, Christian K�nig wrote: > > > > > > > 3) other uses of kmap() must switch to MMU-notifier protection. > > > > > > I would rather suggest to return the page additionally to the pfn from > > > > > > hva_to_pfn() when the function was able to grab a reference to it. > > > > > > > > > > > > When the page is then not available you can't call kmap() on that either. > > > > > > > > > > > > > If the DRM people are okay with SetPageReserved() as a temporary > > > > > > > hack, we can change or remove the WARN in kvm_is_zone_device_page(), > > > > > > > since that is what you are referring to in the commit message. > > > > > > Adding Daniel for additional comments on this, but essentially we have > > > > > > changed quite some infrastructure to make sure that everybody uses > > > > > > VM_PFNMAP to prevent stuff like this from happening. > > > > > > > > > > > > I would really prefer a proper solution in KVM instead. > > > > > Hmm... Now that I think about it that would be > > > > > > > > > > https://lore.kernel.org/kvm/Yc4H+dGfK83BaGpC@xxxxxxxxxx/t/ > > > > > > > > > > Time to resurrect that work. > > > > Ya. I had previously asked David to first eliminated the usage that isn't > > > > protected by mmu_notifiers, but after seeing the resulting complexity, I had a > > > > change of heart[2]. Can you weigh in on the latter thread, specifically my > > > > proposal of using a module param to let the admin opt-in to "unsafe" kmap usage. > > > I don't think that this is something an admin should be able to decide. > > Why not? Assuming the admin has CAP_SYS_ADMIN, they can reboot the host in a > > myriad of ways. The idea with the KVM module param is to allow curated setups > > where the userspace VMM is trusted to a large extent, e.g. AWS' Nitro, to opt-in > > to the unsafe behavior. I.e. by enabling the flag, the admin is acknowledging > > that bad things can happen if untrusted/compromised userspace gets ahold of > > /dev/kvm. > > Well exactly that's the point, you don't need untrusted/compromised > userspace the system could just go spontaneously into nirvana during normal > operation. The proposal is that the module param is off by default, and the expectation is that it would be turned on only by very specific setups. I would not object to making it even more difficult to enable, burying it behind a Kconfig, but IMO that is unnecessary. > This would result in very very hard to debug problems since the issues only > happen rather rarely. > > On the other hand why do you need the kmap() in the first place? The Nitro setup manages guest memory in userspace, and hides that memory from the kernel, e.g. to avoid struct page overhead.. To enable KVM to access guest memory in select flows, e.g. for paravirtualization, without taking on too much complexity, KVM supports kmap() of that memory. For some uses, e.g. nested virtualization, switching to uaccess might be feasible, but the paravirt stuff, especially Xen support, would likely be insanely complex to do without kmap().
