Re: Untested fix for attributes vs. hugepage race

[Sean Christopherson <seanjc@google.com>]

On Thu, Apr 17, 2025, Michael Roth wrote:
> > +	/*
> > +	 * If the head and tail pages of the range currently allow a hugepage,
> > +	 * i.e. reside fully in the slot and don't have mixed attributes, then
> > +	 * add each corresponding hugepage range to the ongoing invalidation,
> > +	 * e.g. to prevent KVM from creating a hugepage in response to a fault
> > +	 * for a gfn whose attributes aren't changing.  Note, only the range
> > +	 * of gfns whose attributes are being modified needs to be explicitly
> > +	 * unmapped, as that will unmap any existing hugepages.
> > +	 */
> > +	for (level = PG_LEVEL_2M; level <= KVM_MAX_HUGEPAGE_LEVEL; level++) {
> > +		gfn_t start = gfn_round_for_level(range->start, level);
> > +		gfn_t nr_pages = KVM_PAGES_PER_HPAGE(level);
> > +		gfn_t end = gfn_round_for_level(range->end, level);
> > +
> > +		if ((start != range->start || start + nr_pages > range->end) &&
> > +		    start >= slot->base_gfn &&
> > +		    start + nr_pages <= slot->base_gfn + slot->npages &&
> > +		    !hugepage_test_mixed(slot, start, level))
> > +			kvm_mmu_invalidate_range_add(kvm, start, start + nr_pages);
> 
> For the 'start + nr_pages > range->end' case, that seems to correspond
> to when the 'start' hugepage covers the end of the range that's being
> invalidated.

It covers the case where range->start is hugepage aligned, but the size of the
range is less than a hugepage.

> But in that case, 'start' and 'end' hugepages are one and the same,

Yes.

> so the below would also trigger,

Gah, that's a goof in the computation of "end".

> and we end up updating the range twice with the same start/end GFN of the
> same hugepage.
>
> Not a big deal, but maybe we should adjust the above logic to only cover
> the case where range->start needs to be extended. Then, if 'start' and
> 'end' hugepages are the same, we are done with that level:

FWIW, this is what I was trying to do.

> 
>     if (start < range->start &&
>         start >= slot->base_gfn &&
>         start + nr_pages <= slot->base_gfn + slot->npages &&
>         !hugepage_test_mixed(slot, start, level))
>             kvm_mmu_invalidate_range_add(kvm, start, start + nr_pages);
> 
>     if (start == end)
>         continue;
> 
> Then what remains to be determined below is whether or not range->end needs
> to be additionally extended by 'end' separate hugepage.
> 
> > +
> > +		if (end < range->end &&
> 
> This seems a little weird since end is almost by definition going to be

Not almost, it is by definition.  But as above, I botched the computation of end.

> less-than or equal-to range->end, so it's basically skipping the equal-to
> case. To avoid needing to filter than case, maybe we should change this:
> 
>   gfn_t end = gfn_round_for_level(range->end, level);
> 
> to
> 
>   gfn_t end = gfn_round_for_level(range->end - 1, level);
> 
> since range->end is non-inclusive and we only care about hugepages that
> begin before the end of the range. If we do that, then 'end < range->end'
> check will always be true and the below:
> 
> > +		if (end < range->end &&
> > +		    (end + nr_pages) <= (slot->base_gfn + slot->npages) &&
> > +		    !hugepage_test_mixed(slot, end, level))
> > +			kvm_mmu_invalidate_range_add(kvm, end, end + nr_pages);
> 
> can be simplified to:
> 
>     if (end + nr_pages <= slot->base_gfn + slot->npages &&
>         !hugepage_test_mixed(slot, end, level))
>             kvm_mmu_invalidate_range_add(kvm, end, end + nr_pages);

That all looks good to me.  And to ensure we don't go off the rails due to bad
inputs (which are supposed to be fully validated by common KVM), we could add a
WARN to detect a non-exclusive range->end.

So this?

	if (WARN_ON_ONCE(range->end <= range->start))
		return false;

	/*
	 * If the head and tail pages of the range currently allow a hugepage,
	 * i.e. reside fully in the slot and don't have mixed attributes, then
	 * add each corresponding hugepage range to the ongoing invalidation,
	 * e.g. to prevent KVM from creating a hugepage in response to a fault
	 * for a gfn whose attributes aren't changing.  Note, only the range
	 * of gfns whose attributes are being modified needs to be explicitly
	 * unmapped, as that will unmap any existing hugepages.
	 */
	for (level = PG_LEVEL_2M; level <= KVM_MAX_HUGEPAGE_LEVEL; level++) {
		gfn_t start = gfn_round_for_level(range->start, level);
		gfn_t end = gfn_round_for_level(range->end - 1, level);
		gfn_t nr_pages = KVM_PAGES_PER_HPAGE(level);

		if ((start != range->start || start + nr_pages > range->end) &&
		    start >= slot->base_gfn &&
		    start + nr_pages <= slot->base_gfn + slot->npages &&
		    !hugepage_test_mixed(slot, start, level))
			kvm_mmu_invalidate_range_add(kvm, start, start + nr_pages);

		if (end == start)
			continue;

		if ((end + nr_pages) <= (slot->base_gfn + slot->npages) &&
		    !hugepage_test_mixed(slot, end, level))
			kvm_mmu_invalidate_range_add(kvm, end, end + nr_pages);
	}