Received-SPF: pass (google.com: domain of linux-kernel+bounces-74685-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Michael Ellerman <mpe@ellerman.id.au>
To: Christophe Leroy <christophe.leroy@csgroup.eu>, Nicholas Piggin
 <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>,
 linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 2/2] powerpc: Don't ignore errors from set_memory_{n}p()
 in __kernel_map_pages()
In-Reply-To: <20ef75884aa6a636e8298736f3d1056b0793d3d9.1708078640.git.christophe.leroy@csgroup.eu>
References: <3656d47c53bff577739dac536dbae31fff52f6d8.1708078640.git.christophe.leroy@csgroup.eu>
 <20ef75884aa6a636e8298736f3d1056b0793d3d9.1708078640.git.christophe.leroy@csgroup.eu>
Date: Wed, 21 Feb 2024 23:09:43 +1100
Message-ID: <878r3eauig.fsf@mail.lhotse>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> set_memory_p() and set_memory_np() can fail.
>
> As mentioned in linux/mm.h:
>
> /*
>  * To support DEBUG_PAGEALLOC architecture must ensure that
>  * __kernel_map_pages() never fails
>  */
>
> So panic in case set_memory_p() or set_memory_np() fail
> in __kernel_map_pages().
>
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/book3s/64/hash.h |  2 +-
>  arch/powerpc/mm/book3s64/hash_utils.c     |  3 ++-
>  arch/powerpc/mm/pageattr.c                | 10 +++++++---
>  3 files changed, 10 insertions(+), 5 deletions(-)
>
..
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 16b8d20d6ca8..62b678585878 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>  #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>  void __kernel_map_pages(struct page *page, int numpages, int enable)
>  {
> +	int err;
>  	unsigned long addr = (unsigned long)page_address(page);
>  
>  	if (PageHighMem(page))
>  		return;
>  
>  	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
> -		hash__kernel_map_pages(page, numpages, enable);
> +		err = hash__kernel_map_pages(page, numpages, enable);
>  	else if (enable)
> -		set_memory_p(addr, numpages);
> +		err = set_memory_p(addr, numpages);
>  	else
> -		set_memory_np(addr, numpages);
> +		err = set_memory_np(addr, numpages);
> +
> +	if (err)
> +		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");

This doesn't compile, it's missing __func__ I guess.

Seems like we could keep it simpler though, it should hopefully never
happen anyway, eg:

  panic("%s: changing memory protections failed\n", __func__);


cheers