Re: [PATCH V2] KVM: SEV: fix wrong pinning of pages

[David Hildenbrand <david@redhat.com>]

On 10.01.25 12:42, yangge1116@126.com wrote:
> From: yangge <yangge1116@126.com>
> 
> When pin_user_pages_fast() pins SEV guest memory without the
> FOLL_LONGTERM flag, the pages will not get migrated out of MIGRATE_CMA/
> ZONE_MOVABLE, violating these mechanisms to avoid fragmentation with
> unmovable pages, for example making CMA allocations fail.
> 
> To address the aforementioned problem, we propose adding the
> FOLL_LONGTERM flag to the pin_user_pages_fast() function.
> 
> Signed-off-by: yangge <yangge1116@126.com>
> ---
> 
> V2:
> - update code and commit message suggested by David
> 
>   arch/x86/kvm/svm/sev.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 943bd07..96f3b8e 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -630,6 +630,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
>   	unsigned long locked, lock_limit;
>   	struct page **pages;
>   	unsigned long first, last;
> +	unsigned int flags = FOLL_LONGTERM;
>   	int ret;
>   
>   	lockdep_assert_held(&kvm->lock);
> @@ -662,8 +663,10 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
>   	if (!pages)
>   		return ERR_PTR(-ENOMEM);
>   
> +	flags |= write ? FOLL_WRITE : 0;
> +
>   	/* Pin the user virtual address. */
> -	npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages);
> +	npinned = pin_user_pages_fast(uaddr, npages, flags, pages);
>   	if (npinned != npages) {
>   		pr_err("SEV: Failure locking %lu pages.\n", npages);
>   		ret = -ENOMEM;

Sorry, looking into it in more detail, there are some paths that immediately unpin again,
and don't seem to have longterm semantics.


Could we do the following, so longterm pinning would be limited to the memory regions
that might stay pinned possible forever?

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 943bd074a5d37..4b0f03f0ea741 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -622,7 +622,7 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
  
  static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
                                     unsigned long ulen, unsigned long *n,
-                                   int write)
+                                   int flags)
  {
         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
         unsigned long npages, size;
@@ -663,7 +663,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
                 return ERR_PTR(-ENOMEM);
  
         /* Pin the user virtual address. */
-       npinned = pin_user_pages_fast(uaddr, npages, write ? FOLL_WRITE : 0, pages);
+       npinned = pin_user_pages_fast(uaddr, npages, flags, pages);
         if (npinned != npages) {
                 pr_err("SEV: Failure locking %lu pages.\n", npages);
                 ret = -ENOMEM;
@@ -751,7 +751,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
         vaddr_end = vaddr + size;
  
         /* Lock the user memory. */
-       inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
+       inpages = sev_pin_memory(kvm, vaddr, size, &npages, FOLL_WRITE);
         if (IS_ERR(inpages))
                 return PTR_ERR(inpages);
  
@@ -1250,7 +1250,8 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
                 if (IS_ERR(src_p))
                         return PTR_ERR(src_p);
  
-               dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n, 1);
+               dst_p = sev_pin_memory(kvm, dst_vaddr & PAGE_MASK, PAGE_SIZE, &n,
+                                      FOLL_WRITE);
                 if (IS_ERR(dst_p)) {
                         sev_unpin_memory(kvm, src_p, n);
                         return PTR_ERR(dst_p);
@@ -1316,7 +1317,8 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
         if (copy_from_user(&params, u64_to_user_ptr(argp->data), sizeof(params)))
                 return -EFAULT;
  
-       pages = sev_pin_memory(kvm, params.guest_uaddr, params.guest_len, &n, 1);
+       pages = sev_pin_memory(kvm, params.guest_uaddr, params.guest_len, &n,
+                              FOLL_WRITE);
         if (IS_ERR(pages))
                 return PTR_ERR(pages);
  
@@ -1798,7 +1800,7 @@ static int sev_receive_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
  
         /* Pin guest memory */
         guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
-                                   PAGE_SIZE, &n, 1);
+                                   PAGE_SIZE, &n, FOLL_WRITE);
         if (IS_ERR(guest_page)) {
                 ret = PTR_ERR(guest_page);
                 goto e_free_trans;
@@ -2696,7 +2698,8 @@ int sev_mem_enc_register_region(struct kvm *kvm,
                 return -ENOMEM;
  
         mutex_lock(&kvm->lock);
-       region->pages = sev_pin_memory(kvm, range->addr, range->size, &region->npages, 1);
+       region->pages = sev_pin_memory(kvm, range->addr, range->size, &region->npages,
+                                      FOLL_WRITE | FOLL_LONGTERM);
         if (IS_ERR(region->pages)) {
                 ret = PTR_ERR(region->pages);
                 mutex_unlock(&kvm->lock);


Thoughts?

-- 
Cheers,

David / dhildenb