Re: [PATCH 2/2] KVM: SVM: Fix an error code in sev_gmem_post_populate()

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/13/24 09:51, Dan Carpenter wrote:
> On Mon, Aug 12, 2024 at 09:04:24PM -0700, Sean Christopherson wrote:
>> On Wed, Jun 12, 2024, Dan Carpenter wrote:
>>> The copy_from_user() function returns the number of bytes which it
>>> was not able to copy.  Return -EFAULT instead.
>>
>> Unless I'm misreading the code and forgetting how all this works, this is
>> intentional.  The direct caller treats any non-zero value as a error:
>>
>> 		ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
>>
>> 		put_page(pfn_to_page(pfn));
>> 		if (ret)
>> 			break;
>> 	}
>>
>> 	filemap_invalidate_unlock(file->f_mapping);
>>
>> 	fput(file);
>> 	return ret && !i ? ret : i;
>>
> 
> No, you're not reading this correctly.  The loop is supposed to return the
> number of pages which were handled successfully.  So this is saying that if the
> first iteration fails and then return a negative error code.  But with the bug
> then if the first iteration fails, it returns the number of bytes which failed.

Yes, you're supposed to return 0 or -errno, so that you return -errno on 
the first round.  Applying the patches, 1/2 is also a bugfix even if the 
printks may be overkill.

Thanks for the report!

Paolo

> The units are wrong pages vs bytes and the failure vs success is reversed.
> 
> Also I notice now that i isn't correct unless we hit a break statement:
> 
> virt/kvm/guest_memfd.c
>     647          npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
> 
> If there isn't enough pages, we use what's available.
> 
>     648          for (i = 0; i < npages; i += (1 << max_order)) {
> 
> If we exit because i >= npages then we return success as if we were able to
> complete one final iteration through the loop.
> 
>     649                  struct folio *folio;
>     650                  gfn_t gfn = start_gfn + i;
>     651                  bool is_prepared = false;
>     652                  kvm_pfn_t pfn;
>     653
>     654                  if (signal_pending(current)) {
>     655                          ret = -EINTR;
>     656                          break;
>     657                  }
>     658
>     659                  folio = __kvm_gmem_get_pfn(file, slot, gfn, &pfn, &is_prepared, &max_order);
>     660                  if (IS_ERR(folio)) {
>     661                          ret = PTR_ERR(folio);
>     662                          break;
>     663                  }
>     664
>     665                  if (is_prepared) {
>     666                          folio_unlock(folio);
>     667                          folio_put(folio);
>     668                          ret = -EEXIST;
>     669                          break;
>     670                  }
>     671
>     672                  folio_unlock(folio);
>     673                  WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
>     674                          (npages - i) < (1 << max_order));
>     675
>     676                  ret = -EINVAL;
>     677                  while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
>     678                                                          KVM_MEMORY_ATTRIBUTE_PRIVATE,
>     679                                                          KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
>     680                          if (!max_order)
>     681                                  goto put_folio_and_exit;
>     682                          max_order--;
>     683                  }
>     684
>     685                  p = src ? src + i * PAGE_SIZE : NULL;
>     686                  ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
>                          ^^^^^^^^^^^^^^^^^^^^
> post_populate() is a pointer to sev_gmem_post_populate() which has is supposed
> to return negative error codes but instead returns number of bytes which failed.
> 
>     687                  if (!ret)
>     688                          kvm_gmem_mark_prepared(folio);
>     689
>     690  put_folio_and_exit:
>     691                  folio_put(folio);
>     692                  if (ret)
>     693                          break;
>     694          }
>     695
>     696          filemap_invalidate_unlock(file->f_mapping);
>     697
>     698          fput(file);
>     699          return ret && !i ? ret : i;
>     700  }
> 
> regards,
> dan carpenter
> 
>