On Wednesday 28 Feb 2024 at 12:11:30 (+0100), David Hildenbrand wrote: > On 28.02.24 11:48, Quentin Perret wrote: > > On Tuesday 27 Feb 2024 at 15:59:37 (+0100), David Hildenbrand wrote: > > > > > > > > Ah, this was something I hadn't thought about. I think both Fuad and I > > > > need to update our series to check the refcount rather than mapcount > > > > (kvm_is_gmem_mapped for Fuad, gunyah_folio_lend_safe for me). > > > > > > An alternative might be !folio_mapped() && !folio_maybe_dma_pinned(). But > > > checking for any unexpected references might be better (there are still some > > > GUP users that don't use FOLL_PIN). > > > > As a non-mm person I'm not sure to understand to consequences of holding > > a GUP pin to a page that is not covered by any VMA. The absence of VMAs > > imply that userspace cannot access the page right? Presumably the kernel > > can't be coerced into accessing that page either? Is that correct? > > Simple example: register the page using an iouring fixed buffer, then unmap > the VMA. iouring now has the page pinned and can read/write it using an > address in the kernel vitual address space (direct map). > > Then, you can happily make the kernel read/write that page using iouring, > even though no VMA still covers/maps that page. Makes sense, and yes that would be a major bug if we let that happen, thanks for the explanation. > [...] > > > > Instead of > > > > > > 1) Converting a page to private only if there are no unexpected > > > references (no mappings, GUP pins, ...) and no VMAs covering it where > > > we could fault it in later > > > 2) Disallowing mmap when the range would contain any private page > > > 3) Handling races between mmap and page conversion > > > > The one thing that makes the second option cleaner from a userspace > > perspective (IMO) is that the conversion to private is happening lazily > > during guest faults. So whether or not an mmapped page can indeed be > > accessed from userspace will be entirely undeterministic as it depends > > on the guest faulting pattern which userspace is entirely unaware of. > > Elliot's suggestion would prevent spurious crashes caused by that > > somewhat odd behaviour, though arguably sane userspace software > > shouldn't be doing that to start with. > > The last sentence is the important one. User space should not access that > memory. If it does, it gets a slap on the hand. Because it should not access > that memory. > > We might even be able to export to user space which pages are currently > accessible and which ones not (e.g., pagemap), although it would be racy as > long as the VM is running and can trigger a conversion. > > > > > To add a layer of paint to the shed, the usage of SIGBUS for > > something that is really a permission access problem doesn't feel > > SIGBUS stands for "BUS error (bad memory access)." > > Which makes sense, if you try accessing something that can no longer be > accessed. It's now inaccessible. Even if it is temporarily. > > Just like a page with an MCE error. Swapin errors. Etc. You cannot access > it. > > It might be a permission problem on the pKVM side, but it's not the > traditional "permission problem" as in mprotect() and friends. You cannot > resolve that permission problem yourself. It's a higher entity that turned > that memory inaccessible. Well that's where I'm not sure to agree. Userspace can, in fact, get back all of that memory by simply killing the protected VM. With the approach suggested here, the guestmem pages are entirely accessible to the host until they are attached to a running protected VM which triggers the protection. It is very much userspace saying "I promise not to touch these pages from now on" when it does that, in a way that I personally find very comparable to the mprotect case. It is not some other entity that pulls the carpet from under userspace's feet, it is userspace being inconsistent with itself that causes the issue here, and that's why SIGBUS feels kinda wrong as it tends to be used to report external errors of some sort. > > appropriate. Allocating memory via guestmem and donating that to a > > protected guest is a way for userspace to voluntarily relinquish access > > permissions to the memory it allocated. So a userspace process violating > > that could, IMO, reasonably expect a SEGV instead of SIGBUS. By the > > point that signal would be sent, the page would have been accounted > > against that userspace process, so not sure the paging examples that > > were discussed earlier are exactly comparable. To illustrate that > > differently, given that pKVM and Gunyah use MMU-based protection, there > > is nothing architecturally that prevents a guest from sharing a page > > back with Linux as RO. > > Sure, then allow page faults that allow for reads and give a signal on write > faults. > > In the scenario, it even makes more sense to not constantly require new > mmap's from user space just to access a now-shared page. > > > Note that we don't currently support this, so I > > don't want to conflate this use case, but that hopefully makes it a > > little more obvious that this is a "there is a page, but you don't > > currently have the permission to access it" problem rather than "sorry > > but we ran out of pages" problem. > > We could user other signals, at least as the semantics are clear and it's > documented. Maybe SIGSEGV would be warranted. > > I consider that a minor detail, though. > > Requiring mmap()/munmap() dances just to access a page that is now shared > from user space sounds a bit suboptimal. But I don't know all the details of > the user space implementation. Agreed, if we could save having to mmap() each page that gets shared back that would be a nice performance optimization. > "mmap() the whole thing once and only access what you are supposed to > access" sounds reasonable to me. If you don't play by the rules, you get a > signal. "... you get a signal, or maybe you don't". But yes I understand your point, and as per the above there are real benefits to this approach so why not. What do we expect userspace to do when a page goes from shared back to being guest-private, because e.g. the guest decides to unshare? Use munmap() on that page? Or perhaps an madvise() call of some sort? Note that this will be needed when starting a guest as well, as userspace needs to copy the guest payload in the guestmem file prior to starting the protected VM. Thanks, Quentin
