linux-kernel - Re: [PATCH v3 12/32] KVM: arm64: Introduce a Hyp buddy page allocator

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YEEBccpDtnJJdwkJ@google.com>
Date:   Thu, 4 Mar 2021 15:49:05 +0000
From:   Quentin Perret <qperret@...gle.com>
To:     Will Deacon <will@...nel.org>
Cc:     catalin.marinas@....com, maz@...nel.org, james.morse@....com,
        julien.thierry.kdev@...il.com, suzuki.poulose@....com,
        android-kvm@...gle.com, linux-kernel@...r.kernel.org,
        kernel-team@...roid.com, kvmarm@...ts.cs.columbia.edu,
        linux-arm-kernel@...ts.infradead.org, tabba@...gle.com,
        mark.rutland@....com, dbrazdil@...gle.com, mate.toth-pal@....com,
        seanjc@...gle.com, robh+dt@...nel.org
Subject: Re: [PATCH v3 12/32] KVM: arm64: Introduce a Hyp buddy page allocator

On Thursday 04 Mar 2021 at 15:30:36 (+0000), Will Deacon wrote:
> On Tue, Mar 02, 2021 at 02:59:42PM +0000, Quentin Perret wrote:
> > +void *hyp_alloc_pages(struct hyp_pool *pool, unsigned int order)
> > +{
> > +	unsigned int i = order;
> > +	struct hyp_page *p;
> > +
> > +	hyp_spin_lock(&pool->lock);
> > +
> > +	/* Look for a high-enough-order page */
> > +	while (i < pool->max_order && list_empty(&pool->free_area[i]))
> > +		i++;
> > +	if (i >= pool->max_order) {
> > +		hyp_spin_unlock(&pool->lock);
> > +		return NULL;
> > +	}
> > +
> > +	/* Extract it from the tree at the right order */
> > +	p = list_first_entry(&pool->free_area[i], struct hyp_page, node);
> > +	p = __hyp_extract_page(pool, p, order);
> > +
> > +	hyp_spin_unlock(&pool->lock);
> > +	hyp_page_ref_inc(p);
> 
> I find this a little scary, as we momentarily drop the lock. It think
> it's ok because the reference count on the page must be 0 at this point,

Yep, @p shouldn't be visible to the caller yet so this should be fine.

> but actually then I think it would be clearer to have a
> hyp_page_ref_init() function which could take the lock, check that the
> refcount is indeed 0 and then set it to 1.

Works for me. Maybe I'll use another name for the API to stay consistent
with the kernel gfp code (hyp_page_ref_inc() and friends are inspired
from their kernel counterpart). And I guess I can hyp_panic() if the
refcount is not 0 at this point to match the VM_BUG_ON_PAGE() in
set_page_refcounted().

Thanks!
Quentin