On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita kalyazin@amazon.co.uk wrote:

From: Nikita Kalyazin kalyazin@amazon.com

Hi Nikita,

write syscall populates guest_memfd with user-supplied data in a generic way, ie no vendor-specific preparation is performed. This is supposed to be used in non-CoCo setups where guest memory is not hardware-encrypted.

What's meant to happen if we do use this for CoCo VMs? I would expect write() to fail, but I don't see why it would (seems like we need/want a check that we aren't write()ing to private memory).

The following behaviour is implemented:

• only page-aligned count and offset are allowed
• if the memory is already allocated, the call will successfully populate it
• if the memory is not allocated, the call will both allocate and populate
• if the memory is already populated, the call will not repopulate it

Signed-off-by: Nikita Kalyazin kalyazin@amazon.com

virt/kvm/guest_memfd.c | 64 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index 08a6bc7d25b6..a2e86ec13e4b 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -379,7 +379,9 @@ static int kvm_gmem_mmap(struct file *file, struct vm_area_struct *vma) }

static struct file_operations kvm_gmem_fops = {

•   .mmap           = kvm_gmem_mmap,
  

•   .mmap           = kvm_gmem_mmap,
  

•   .llseek         = default_llseek,
  

•   .write_iter     = generic_perform_write,
  

You seem to have accidentally replaced some tabs with spaces here. :) Please keep the style consistent.

    .open           = generic_file_open,
    .release        = kvm_gmem_release,
    .fallocate      = kvm_gmem_fallocate,

@@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module) kvm_gmem_fops.owner = module; }

+static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,

•                                struct address_space *mapping,
  

•                                loff_t pos, unsigned int len,
  

•                                struct folio **foliop,
  

•                                void **fsdata)
  

+{

•   struct file *file = kiocb->ki_filp;
  

•   pgoff_t index = pos >> PAGE_SHIFT;
  

•   struct folio *folio;
  

•   if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
  

•           return -EINVAL;
  

Requiring pos to be page-aligned seems like a strange restriction, and requiring len to be exactly PAGE_SIZE just seems wrong. I don't see any reason why the below logic can't be made to work with an unrestricted pos and len (in other words, I don't see how guest_memfd is special vs other filesystems in this regard).

•   if (pos + len > i_size_read(file_inode(file)))
  

•           return -EINVAL;
  

•   folio = kvm_gmem_get_folio(file_inode(file), index);
  

•   if (IS_ERR(folio))
  

•           return -EFAULT;
  

•   if (WARN_ON_ONCE(folio_test_large(folio))) {
  

•           folio_unlock(folio);
  

•           folio_put(folio);
  

•           return -EFAULT;
  

•   }
  

•   if (folio_test_uptodate(folio)) {
  

•           folio_unlock(folio);
  

•           folio_put(folio);
  

•           return -ENOSPC;
  

Does it actually matter for the folio not to be uptodate? It seems unnecessarily restrictive not to be able to overwrite data if we're saying that this is only usable for unencrypted memory anyway.

Is ENOSPC really the right errno for this? (Maybe -EFAULT?)

•   }
  

•   *foliop = folio;
  

•   return 0;
  

+}

+static int kvm_kmem_gmem_write_end(const struct kiocb *kiocb,

•                              struct address_space *mapping,
  

•                              loff_t pos, unsigned int len,
  

•                              unsigned int copied,
  

•                              struct folio *folio, void *fsdata)
  

+{

•   if (copied) {
  

•           if (copied < len) {
  

•                   unsigned int from = pos & (PAGE_SIZE - 1);
  

How about:

unsigned int from = pos & ((1UL << folio_order(*folio)) - 1)

So that we don't need to require !folio_test_large() in kvm_kmem_gmem_write_begin().

•                   folio_zero_range(folio, from + copied, len - copied);
  

•           }
  

•           kvm_gmem_mark_prepared(folio);
  

•   }
  

•   folio_unlock(folio);
  

•   folio_put(folio);
  

•   return copied;
  

+}

static int kvm_gmem_migrate_folio(struct address_space *mapping, struct folio *dst, struct folio *src, enum migrate_mode mode) @@ -442,6 +501,8 @@ static void kvm_gmem_free_folio(struct folio *folio)

static const struct address_space_operations kvm_gmem_aops = { .dirty_folio = noop_dirty_folio,

•   .write_begin = kvm_kmem_gmem_write_begin,
  

•   .write_end = kvm_kmem_gmem_write_end,
    .migrate_folio  = kvm_gmem_migrate_folio,
    .error_remove_folio = kvm_gmem_error_folio,
  

#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE @@ -489,6 +550,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags) }

    file->f_flags |= O_LARGEFILE;

•   file->f_mode |= FMODE_LSEEK | FMODE_PWRITE;
  
    inode = file->f_inode;
    WARN_ON(file->f_mapping != inode->i_mapping);
  

-- 2.50.1

On Thu, Sep 11, 2025 at 3:15 AM Nikita Kalyazin kalyazin@amazon.com wrote:

On 10/09/2025 22:23, James Houghton wrote:

...

On Tue, Sep 2, 2025 at 4:20 AM Kalyazin, Nikita kalyazin@amazon.co.uk wrote:

...

From: Nikita Kalyazin kalyazin@amazon.com

Hi Nikita,

Hi James,

Thanks for the review!

:) I hope it's actually helpful.

...

...

write syscall populates guest_memfd with user-supplied data in a generic way, ie no vendor-specific preparation is performed. This is supposed to be used in non-CoCo setups where guest memory is not hardware-encrypted.

What's meant to happen if we do use this for CoCo VMs? I would expect write() to fail, but I don't see why it would (seems like we need/want a check that we aren't write()ing to private memory).

I am not so sure that write() should fail even in CoCo VMs if we access not-yet-prepared pages. My understanding was that the CoCoisation of the memory occurs during "preparation". But I may be wrong here.

This sounds fine to me, but could you update the changelog with what the behavior is for CoCo VMs and why we don't allow writing to the same pages/regions twice? Something like:

"Although write() is only meant to be used for non-CoCo VMs, it is valid for CoCo VMs as well: the written contents will be encrypted (when the page is prepared). Because the contents may be encrypted, it is nonsensical to write() again, so write()ing to prepared pages is disallowed (even if the no memory encryption occurs). Furthermore, in the near future, page preparation will also result in pages being removed from the direct map, so there will be no direct map through which to perform the second write()."

(This is all provided that it's actually okay to write() content that will be encrypted... I don't know why that would be improper, but I'm not exactly an expert here.)

...

...

@@ -390,6 +392,63 @@ void kvm_gmem_init(struct module *module) kvm_gmem_fops.owner = module; }

+static int kvm_kmem_gmem_write_begin(const struct kiocb *kiocb,

•                                struct address_space *mapping,
  

•                                loff_t pos, unsigned int len,
  

•                                struct folio **foliop,
  

•                                void **fsdata)
  

+{

•   struct file *file = kiocb->ki_filp;
  

•   pgoff_t index = pos >> PAGE_SHIFT;
  

•   struct folio *folio;
  

•   if (!PAGE_ALIGNED(pos) || len != PAGE_SIZE)
  

•           return -EINVAL;
  

Requiring pos to be page-aligned seems like a strange restriction, and requiring len to be exactly PAGE_SIZE just seems wrong. I don't see any reason why the below logic can't be made to work with an unrestricted pos and len (in other words, I don't see how guest_memfd is special vs other filesystems in this regard).

I don't have a real reason to apply those restrictions. Happy to remove them, thanks.

Thanks! Presumably you'll make it so that any unaligned segments will be left as zeroes; please describe this in the changelog as well. :)

...

...

•   if (pos + len > i_size_read(file_inode(file)))
  

•           return -EINVAL;
  

•   folio = kvm_gmem_get_folio(file_inode(file), index);
  

•   if (IS_ERR(folio))
  

•           return -EFAULT;
  

•   if (WARN_ON_ONCE(folio_test_large(folio))) {
  

•           folio_unlock(folio);
  

•           folio_put(folio);
  

•           return -EFAULT;
  

•   }
  

•   if (folio_test_uptodate(folio)) {
  

•           folio_unlock(folio);
  

•           folio_put(folio);
  

•           return -ENOSPC;
  

Does it actually matter for the folio not to be uptodate? It seems unnecessarily restrictive not to be able to overwrite data if we're saying that this is only usable for unencrypted memory anyway.

In the context of direct map removal [1] it does actually because when we mark a folio as prepared, we remove it from the direct map making it inaccessible to the way write() performs the copy. It does not matter if direct map removal isn't enabled though. Do you think it should be conditional?

Oh, good point. It's simpler (both to implement and to describe) to disallow a second write() call in all cases (no matter if the direct map for the page has been removed or if the contents have been encrypted), so I'm all for leaving it unconditional like you have now. Thanks!

...

Is ENOSPC really the right errno for this? (Maybe -EFAULT?)

I don't have a strong opinion here. My reasoning was if the folio is already "sealed" by the direct map removal, then it is no longer a part of the "writable space", so -ENOSPC makes sense. Maybe this intuition only works for me so I'm happy to change to -EFAULT if it looks less confusing.

Oh actually.... how about EEXIST? That feels like the most natural. But also no strong opinion here.

Thanks for all the clarification, Nikita. :)