Re: [PATCH] KVM: SEV: drop FOLL_LONGTERM for encrypted region registration

[Gupta, Pankaj]

> > Hi David,
> >
> > > > commit 7e066cb9b71a ("KVM: SEV: Use long-term pin when registering encrypted
> > > > memory regions")
> > > > added FOLL_LONGTERM to sev_mem_enc_register_region() so anonymous guest RAM is
> > > > migrated out of MIGRATE_CMA/ZONE_MOVABLE before a long term pin. This breaks
> > > > virtio-pmem which has file backed (MAP_SHARED) host mapping where GUP rejects
> > > > FOLL_WRITE | FOLL_LONGTERM since:
> > > >
> > > > commit 8ac268436e6d ("mm/gup: disallow FOLL_LONGTERM GUP-nonfast writing to
> > > > file-backed mappings")
> > > > commit a6e79df92e4a ("mm/gup: disallow FOLL_LONGTERM GUP-fast writing to
> > > > file-backed mappings").
> > > >
> > > > Drop FOLL_LONGTERM when registering encrypted memory regions and restore
> > > > the previous behavior.
> > > But that breaks the original issue of breaking ZONE_MOVABLE/CMA?
> > >
> > > If it is a longterm pin, it must use FOLL_LONGTERM. :/
> > >
> > > I assume we fail in check_vma_flags()
> > >
> > >      if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
> > >          return -EOPNOTSUPP;
> > Yes, it fails in this path but for file backed mapping, vma_is_fsdax() returns
> > false because
> >
> > vma_is_dax() returns false:
> Ah, okay, so fsdax is not involved and we really only fail because of the
> writable_file_mapping_allowed() check.
>
> I was for a second thinking in terms of nested virt :)
>
> > > IIRC, fsdax cannot tolerate unbounded pins. Is that the case we are running into?
> > Host side backend is regular file backed memory (no fsdax).
> Okay, so we'll end up mapping an ordinary file into VM memory, and expose that
> to the VM as part of virtio-pmem device.
>
> That also means that vfio etc. won't be able to longterm-pin such device memory.
> So this is not a problem isolated to SEV.
>
> Forbidding to longterm pin is actually the right thing to do if the filesystem
> relies on writenotify, as spelled out by Lorenzo's commit:
>
> "
>      Writing to file-backed mappings which require folio dirty tracking using
>      GUP is a fundamentally broken operation, as kernel write access to GUP
>      mappings do not adhere to the semantics expected by a file system.
>
>      A GUP caller uses the direct mapping to access the folio, which does not
>      cause write notify to trigger, nor does it enforce that the caller marks
>      the folio dirty.
>
>      The problem arises when, after an initial write to the folio, writeback
>      results in the folio being cleaned and then the caller, via the GUP
>      interface, writes to the folio again.
> "
>
> Hmmm

Yes. For file based mapping we don't allow long term pinning.

If we take into account the fragmentation concerns for MIGRATE_CMA and 
ZONE_MOVABLE allocations

solvable with FOLL_LONGTERM, I can think of two options(tested) to allow 
file based mappings as well:

1. Fallback on FOLL_WRITE when FOLL_LONGTERM fails as suggested by Sean.

2. Explicitly restrict long-term pinning for file-backed mappings with a 
change like the patch below [1].

David, Sean,

Do you have a preference between these two approaches? I am leaning 
toward towards option 2.

Thank you!

Pankaj

---

[1]

arch/x86/kvm/svm/sev.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 6c6a6d663e29..c4b53700f69e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2743,6 +2743,29 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void 
__user *argp)
        return r;
 }

+static unsigned int sev_region_gup_flags(unsigned long uaddr, unsigned 
long ulen)
+{
+       struct mm_struct *mm = current->mm;
+       unsigned long end = uaddr + ulen;
+       struct vm_area_struct *vma;
+       unsigned int flags = FOLL_WRITE | FOLL_LONGTERM;
+       VMA_ITERATOR(vmi, mm, uaddr);
+
+       if (ulen == 0 || end < uaddr)
+               return FOLL_WRITE;
+
+       mmap_read_lock(mm);
+       for_each_vma_range(vmi, vma, end) {
+               if (!vma_is_anonymous(vma)) {
+                       flags = FOLL_WRITE;
+                       break;
+               }
+       }
+       mmap_read_unlock(mm);
+
+       return flags;
+}
+
 int sev_mem_enc_register_region(struct kvm *kvm,
                                struct kvm_enc_region *range)
 {
@@ -2764,7 +2787,7 @@ int sev_mem_enc_register_region(struct kvm *kvm,
                return -ENOMEM;

        region->pages = sev_pin_memory(kvm, range->addr, range->size, 
&region->npages,
-                                      FOLL_WRITE | FOLL_LONGTERM);
+ sev_region_gup_flags(range->addr, range->size));
        if (IS_ERR(region->pages)) {
                ret = PTR_ERR(region->pages);
                goto e_free;
(END)
--