Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c45a6850-772e-8d2c-9261-7621f242dbae@redhat.com>
Date:   Tue, 21 Jun 2022 10:01:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH] mm: fixup validation of buddy pfn
Content-Language: en-US
To:     Xianting Tian <xianting.tian@linux.alibaba.com>,
        akpm@linux-foundation.org, ziy@nvidia.com
Cc:     guoren@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20220621031118.3650529-1-xianting.tian@linux.alibaba.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <20220621031118.3650529-1-xianting.tian@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 21.06.22 05:11, Xianting Tian wrote:
> For RISC-V arch the first 2MB RAM could be reserved for opensbi,
> and the arch code may don't create pages for the first 2MB RAM,
> so it would have pfn_base=512 and mem_map began with 512th PFN when
> CONFIG_FLATMEM=y.
> 
> But __find_buddy_pfn algorithm thinks the start PFN 0, it could get
> 0 PFN or less than the pfn_base value, so page_is_buddy() can't
> verify the page whose PFN is 0 ~ 511, actually we don't have valid
> pages for PFN 0 ~ 511.
> 
> Actually, buddy system should not assume Arch cretaed pages for
> reserved memory, Arch may don't know the implied limitation.

Ehm, sorry, no. Archs have to stick to the rules of the buddy, not the
other way around. Why should we add additional overhead to the buddy
just because arch XYZ wants to be special?

If at all, we should fail hard if an arch doesn't play with the rules
and make this a VM_BUG_ON().

> With this patch, we can gurantee a valid buddy no matter what we
> have pages for reserved memory or not.
> 
> Fixes: 8170ac4700d26f65 ("mm: wrap __find_buddy_pfn() with a necessary buddy page validation")
> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
> ---
>  mm/internal.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index c0f8fbe0445b..0ec446caeb2e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -322,7 +322,8 @@ __find_buddy_pfn(unsigned long page_pfn, unsigned int order)
>   * The found buddy can be a non PageBuddy, out of @page's zone, or its order is
>   * not the same as @page. The validation is necessary before use it.
>   *
> - * Return: the found buddy page or NULL if not found.
> + * Return: the found buddy page or NULL if not found or NULL if buddy pfn is
> + *         not valid.
>   */
>  static inline struct page *find_buddy_page_pfn(struct page *page,
>  			unsigned long pfn, unsigned int order, unsigned long *buddy_pfn)
> @@ -330,6 +331,9 @@ static inline struct page *find_buddy_page_pfn(struct page *page,
>  	unsigned long __buddy_pfn = __find_buddy_pfn(pfn, order);
>  	struct page *buddy;
>  
> +	if (!pfn_valid(__buddy_pfn))
> +		return NULL;
> +
>  	buddy = page + (__buddy_pfn - pfn);
>  	if (buddy_pfn)
>  		*buddy_pfn = __buddy_pfn;


-- 
Thanks,

David / dhildenb