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
