linux-kernel - Re: [PATCH v3 3/5] Drivers: hv: Batch GPA unmap operations to improve large region performance

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: <aOPZfZV6uLOePaZo@skinsburskii.localdomain>
Date: Mon, 6 Oct 2025 08:00:13 -0700
From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
	"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
	"wei.liu@...nel.org" <wei.liu@...nel.org>,
	"decui@...rosoft.com" <decui@...rosoft.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/5] Drivers: hv: Batch GPA unmap operations to
 improve large region performance

On Fri, Oct 03, 2025 at 09:41:32PM +0000, Michael Kelley wrote:
> From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com> Sent: Friday, October 3, 2025 9:52 AM
> 
> > 
> > On Fri, Oct 03, 2025 at 12:27:13AM +0000, Michael Kelley wrote:
> > > From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com> Sent: Thursday, October 2, 2025 9:36 AM
> > > >
> > > > Reduce overhead when unmapping large memory regions by batching GPA unmap
> > > > operations in 2MB-aligned chunks.
> > > >
> > > > Use a dedicated constant for batch size to improve code clarity and
> > > > maintainability.
> > > >
> > > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>

<snip>

> > > > +					MSHV_MAX_UNMAP_GPA_PAGES, unmap_flags);
> > > > +
> > > > +		page_offset += MSHV_MAX_UNMAP_GPA_PAGES - 1;
> > >
> > > This update to the page_offset doesn't take into account the effect of the
> > > ALIGN (or ALIGN_DOWN) call.  With a change to ALIGN_DOWN(), it may
> > > increment too far and perhaps cause the "for" loop to be exited prematurely,
> > > which would fail to unmap some of the pages.
> > >
> > 
> > I’m not sure I see the problem here.  If we align the offset by
> > MSHV_MAX_UNMAP_GPA_PAGES and unmap the same number of pages, then we
> > should increment the offset by that very same number, shouldn’t we?
> 
> Here's an example showing the problem I see (assuming ALIGN_DOWN
> instead of ALIGN):
> 
> 1) For simplicity in the example, assume region->start_gfn is zero.
> 2) Entries 0 thru 3 (i.e., 4 entries) in region->pages[] are zero.
> 3) Entries 4 thru 515 (the next 512 entries) are non-zero.
> 4) Entries 516 thru 1023 (the next 508 entries) are zero.
> 5) Entries 1024 thru 1535 (the last 512 entries) are non-zero.
> 
> Upon entering the "for" loop for the first time, page_offset gets
> incremented to 4 because of skipping entries 0 thru 3 that are zero.
> On the next iteration where page_offset is 4, the hypercall is made,
> passing 0 for the gfn (because of ALIGN_DOWN), with a count of
> 512, so entries 0 thru 511 are unmapped. Entries 0 thru 3 are valid
> entries, and the fact that they aren't mapped is presumably ignored
> by the hypercall, so everything works.
> 
> Then page_offset is incremented by 511, so it will be 515. Continuing
> the "for" loop increments page_offset to 516. The zero entries 516
> thru 1023 increment page_offset to 1024. Finally the hypercall is made
> again covering entries 1024 thru 1535, none of which are zero.
> 
> But notice that entries 512 thru 515 (which are non-zero) got skipped.
> That's because the first invocation of the hypercall covered only through
> entry 511, while page_offset was incremented to 515. page_offset
> should have been set to 511, since that's the last entry processed by
> the first invocation of the hypercall.
> 
> Michael

I see the problem now. Thank you.
I'll fix it in the next revision.

Thanks,
Stanislav