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

["Gupta, Pankaj" <pankaj.gupta@amd.com>]

>> 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)
--