Re: [PATCH v13 10/24] gunyah: vm_mgr: Add/remove user memory regions

[Will Deacon <will@kernel.org>]

Hi Elliot,

[+Quentin since he's looked at the MMU notifiers]

Sorry for the slow response, I got buried in email during a week away.

On Fri, May 19, 2023 at 10:02:29AM -0700, Elliot Berman wrote:
> On 5/19/2023 4:59 AM, Will Deacon wrote:
> > On Tue, May 09, 2023 at 01:47:47PM -0700, Elliot Berman wrote:
> > > +	ret = account_locked_vm(ghvm->mm, mapping->npages, true);
> > > +	if (ret)
> > > +		goto free_mapping;
> > > +
> > > +	mapping->pages = kcalloc(mapping->npages, sizeof(*mapping->pages), GFP_KERNEL_ACCOUNT);
> > > +	if (!mapping->pages) {
> > > +		ret = -ENOMEM;
> > > +		mapping->npages = 0; /* update npages for reclaim */
> > > +		goto unlock_pages;
> > > +	}
> > > +
> > > +	gup_flags = FOLL_LONGTERM;
> > > +	if (region->flags & GH_MEM_ALLOW_WRITE)
> > > +		gup_flags |= FOLL_WRITE;
> > > +
> > > +	pinned = pin_user_pages_fast(region->userspace_addr, mapping->npages,
> > > +					gup_flags, mapping->pages);
> > > +	if (pinned < 0) {
> > > +		ret = pinned;
> > > +		goto free_pages;
> > > +	} else if (pinned != mapping->npages) {
> > > +		ret = -EFAULT;
> > > +		mapping->npages = pinned; /* update npages for reclaim */
> > > +		goto unpin_pages;
> > > +	}
> > 
> > Sorry if I missed it, but I still don't see where you reject file mappings
> > here.
> > 
> 
> Sure, I can reject file mappings. I didn't catch that was the ask previously
> and thought it was only a comment about behavior of file mappings.

I thought the mention of filesystem corruption was clear enough! It's
definitely something we shouldn't allow.

> > This is also the wrong interface for upstream. Please get involved with
> > the fd-based guest memory discussions [1] and port your series to that.
> > 
> 
> The user interface design for *shared* memory aligns with
> KVM_SET_USER_MEMORY_REGION.

I don't think it does. For example, file mappings don't work (as above),
you're placing additional rlimit requirements on the caller, read-only
memslots are not functional, the memory cannot be swapped or migrated,
dirty logging doesn't work etc. pKVM is in the same boat, but that's why
we're not upstreaming this part in its current form.

> I understood we want to use restricted memfd for giving guest-private memory
> (Gunyah calls this "lending memory"). When I went through the changes, I
> gathered KVM is using restricted memfd only for guest-private memory and not
> for shared memory. Thus, I dropped support for lending memory to the guest
> VM and only retained the shared memory support in this series. I'd like to
> merge what we can today and introduce the guest-private memory support in
> tandem with the restricted memfd; I don't see much reason to delay the
> series.

Right, protected guests will use the new restricted memfd ("guest mem"
now, I think?), but non-protected guests should implement the existing
interface *without* the need for the GUP pin on guest memory pages. Yes,
that means full support for MMU notifiers so that these pages can be
managed properly by the host kernel. We're working on that for pKVM, but
it requires a more flexible form of memory sharing over what we currently
have so that e.g. the zero page can be shared between multiple entities.

Will