linux-kernel - Re: [PATCH V2 kvm-next] KVM: guest_memfd: use kvm_gmem_get_index() in more places and smaller cleanups

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <57e1b0de-00d4-4137-a56b-5135ece6736e@amd.com>
Date: Sat, 11 Oct 2025 23:34:36 +0530
From: "Garg, Shivank" <shivankg@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: pbonzini@...hat.com, david@...hat.com, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH V2 kvm-next] KVM: guest_memfd: use kvm_gmem_get_index() in
 more places and smaller cleanups



On 10/10/2025 10:57 PM, Sean Christopherson wrote:
> TL;DR: Please split this into three patches, call out the use of
> kvm_gmem_get_index() in kvm_gmem_prepare_folio, and unless someone feels strongly
> about the ULONG_MAX change, just drop it.
> 
> On Tue, Sep 02, 2025, Shivank Garg wrote:
>> Move kvm_gmem_get_index() to the top of the file and make it available for
>> use in more places.
> 
> Not just "in more places", specifically for kvm_gmem_prepare_folio().  And this
> also has kvm_gmem_prepare_folio() _use_ the helper.  That detail matters, because
> without having actual user, such code movement would be completely arbitrary and
> likely pointless churn.  E.g. AFAICT, it's not needed for the NUMA support or
> even for the WIP-but-functional in-place conversion patches I have.
> 
>> Remove redundant initialization of the gmem variable because it's already
>> initialized.
>>
>> Replace magic number -1UL with ULONG_MAX.
> 
> This is quite clearly three distinct patches.  Yes, they're trivial, but that's
> exactly why they should be split up: it takes so, so little brain power to review
> super trivial patches.  Bundling such patches together almost always increases
> the total review cost relative to if they are split up.  I.e. if split, the cost
> is A + B + C, but bundled together, the cost is A + B + C + X, where 'X' is the
> extra effort it takes to figure out what changes go with what part of the changelog.
> And sometimes (and for me, it's the case here), X > A + B + C, which makes for
> grumpy reviewers.
> 
> Case in point, it took me way too long to spot the new use of kvm_gmem_get_index()
> in kvm_gmem_prepare_folio(), due to the noise from the other changes getting in
> the way.
> 
> More importantly, bundling things together like this makes it an all-or-nothing
> proposition.  That matters, because I don't want to take the ULONG_MAX change.
> The -1 pattern is meaningful (at least, IMO), as KVM is very specifically
> invalidating 0 => 0xffffffff_ffffffff.  I don't love hiding those details behind
> ULONG_MAX.  I realize it's a somewhat silly position, because xarray uses ULONG_MAX
> for it's terminal value, but it gets weird in the guest_memfd code because @end is
> used for both the xarray and for gfn range sent over to KVM.
> 
> Amusingly, the -1UL is also technically wrong, because @end is exclusive.  AFAIK
> it's not actually possible to populate offset -1, so it's a benign off-by-one,
> but I think super duper technically, we would want something absurd like this:
> 
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index cfbb2f1aa1ab..f4d15cda2029 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -231,12 +231,13 @@ static void __kvm_gmem_invalidate_begin(struct gmem_file *f, pgoff_t start,
>                                         pgoff_t end,
>                                         enum kvm_gfn_range_filter attr_filter)
>  {
> +       pgoff_t last  = end == -1UL ? ULONG_MAX : end;
>         bool flush = false, found_memslot = false;
>         struct kvm_memory_slot *slot;
>         struct kvm *kvm = f->kvm;
>         unsigned long index;
>  
> -       xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
> +       xa_for_each_range(&f->bindings, index, slot, start, last) {
>                 pgoff_t pgoff = slot->gmem.pgoff;
>  
>                 struct kvm_gfn_range gfn_range = {
> 


Thanks for the detailed feedback and review, Sean.
I didn't think enough about this from a reviewer/maintainer's perspective.
I'll split this up, make suggested changes, drop the ULONG_MAX
change, and rebase on kvm-x86 gmem for v3.

Thanks again,
Shivank