[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d8265f6d-8867-4c1a-b715-70bd8c5fb2b2@redhat.com>
Date: Tue, 13 Aug 2024 12:05:43 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Dan Carpenter <dan.carpenter@...aro.org>,
Sean Christopherson <seanjc@...gle.com>
Cc: error27@...il.com, Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>, Brijesh Singh <brijesh.singh@....com>,
Michael Roth <michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] KVM: SVM: Fix an error code in
sev_gmem_post_populate()
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
>
>
Powered by blists - more mailing lists