linux-kernel - Re: [RFC PATCH 4/4] mm: Add merge page notifier

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: <33d14370-b47d-5ceb-09c4-41f0d6b33af8@intel.com>
Date:   Mon, 4 Feb 2019 11:40:32 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     rkrcmar@...hat.com, alexander.h.duyck@...ux.intel.com,
        x86@...nel.org, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        pbonzini@...hat.com, tglx@...utronix.de, akpm@...ux-foundation.org
Subject: Re: [RFC PATCH 4/4] mm: Add merge page notifier

> +void __arch_merge_page(struct zone *zone, struct page *page,
> +		       unsigned int order)
> +{
> +	/*
> +	 * The merging logic has merged a set of buddies up to the
> +	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
> +	 * advantage of this moment to notify the hypervisor of the free
> +	 * memory.
> +	 */
> +	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
> +		return;
> +
> +	/*
> +	 * Drop zone lock while processing the hypercall. This
> +	 * should be safe as the page has not yet been added
> +	 * to the buddy list as of yet and all the pages that
> +	 * were merged have had their buddy/guard flags cleared
> +	 * and their order reset to 0.
> +	 */
> +	spin_unlock(&zone->lock);
> +
> +	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
> +		       PAGE_SIZE << order);
> +
> +	/* reacquire lock and resume freeing memory */
> +	spin_lock(&zone->lock);
> +}

Why do the lock-dropping on merge but not free?  What's the difference?

This makes me really nervous.  You at *least* want to document this at
the arch_merge_page() call-site, and perhaps even the __free_one_page()
call-sites because they're near where the zone lock is taken.

The place you are calling arch_merge_page() looks OK to me, today.  But,
it can't get moved around without careful consideration.  That also
needs to be documented to warn off folks who might move code around.

The interaction between the free and merge hooks is also really
implementation-specific.  If an architecture is getting order-0
arch_free_page() notifications, it's probably worth at least documenting
that they'll *also* get arch_merge_page() notifications.

The reason x86 doesn't double-hypercall on those is not broached in the
descriptions.  That seems to be problematic.