Re: [PATCH 1/5] mm: Add support for unaccepted memory

[Dave Hansen]

On 8/12/21 1:19 AM, Joerg Roedel wrote:
> On Tue, Aug 10, 2021 at 02:20:08PM -0700, Andi Kleen wrote:
>> Also I agree with your suggestion that we should get the slow path out of
>> the zone locks/interrupt disable region. That should be easy enough and is
>> an obvious improvement.
> 
> I also agree that the slow-path needs to be outside of the memory
> allocator locks. But I think this conflicts with the concept of
> accepting memory in 2MB chunks even if allocation size is smaller.
> 
> Given some kernel code allocated 2 pages and the allocator path starts
> to validate the whole 2MB page the memory is on, then there are
> potential races to take into account.

Yeah, the PageOffline()+PageBuddy() trick breaks down as soon as
PageBuddy() gets cleared.

I'm not 100% sure we need a page flag, though.  Imagine if we just did a
static key check in prep_new_page():

	if (static_key_whatever(tdx_accept_ongoing))
		maybe_accept_page(page, order);

maybe_accept_page() could just check the acceptance bitmap and see if
the 2MB page has been accepted.  If so, just return.  If not, take the
bitmap lock, accept the 2MB page, then mark the bitmap.

maybe_accept_page()
{
	unsigned long huge_pfn = page_to_phys(page) / PMD_SIZE;

	/* Test the bit before taking any locks: */
	if (test_bit(huge_pfn, &accepted_bitmap))
		return;

	spin_lock_irq();
	/* Retest inside the lock: */
	if (test_bit(huge_pfn, &accepted_bitmap))
		return;
	tdx_accept_page(page, PMD_SIZE);
	set_bit(huge_pfn, &accepted_bitmap));
	spin_unlock_irq();
}

That's still not great.  It's still a global lock and the lock is still
held for quite a while because that accept isn't going to be lightning
fast.  But, at least it's not holding any *other* locks.  It also
doesn't take any locks in the fast path where the 2MB page was already
accepted.

The locking could be more fine-grained, for sure.  The bitmap could, for
instance, have a lock bit too.  Or we could just have an array of locks
and hash the huge_pfn to find a lock given a huge_pfn.  But, for now, I
think it's fine to just keep the global lock.

> Either some other code path allocates memory from that page and returns
> it before validation is finished or we end up with double validation.
> Returning unvalidated memory is a guest-problem and double validation
> will cause security issues for SNP guests.

Yeah, I think the *canonical* source of information for accepts is the
bitmap.  The page flags and any static keys or whatever are
less-canonical sources that tell you when you _might_ need to consult
the bitmap.