Received-SPF: pass (google.com: domain of linux-kernel+bounces-75625-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Michael Ellerman <mpe@ellerman.id.au>
To: Christophe Leroy <christophe.leroy@csgroup.eu>, Nicholas Piggin
 <npiggin@gmail.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "linuxppc-dev@lists.ozlabs.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: <eefeb056-bc71-46ad-90b3-b07c392f5903@csgroup.eu>
References: <3656d47c53bff577739dac536dbae31fff52f6d8.1708078640.git.christophe.leroy@csgroup.eu>
 <20ef75884aa6a636e8298736f3d1056b0793d3d9.1708078640.git.christophe.leroy@csgroup.eu>
 <878r3eauig.fsf@mail.lhotse>
 <eefeb056-bc71-46ad-90b3-b07c392f5903@csgroup.eu>
Date: Thu, 22 Feb 2024 09:59:35 +1100
Message-ID: <875xyhbezs.fsf@mail.lhotse>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 21/02/2024 =C3=A0 13:09, Michael Ellerman a =C3=A9crit=C2=A0:
>> 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 nu=
mpages, long action)
>>>   #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>>   void __kernel_map_pages(struct page *page, int numpages, int enable)
>>>   {
>>> +	int err;
>>>   	unsigned long addr =3D (unsigned long)page_address(page);
>>>=20=20=20
>>>   	if (PageHighMem(page))
>>>   		return;
>>>=20=20=20
>>>   	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>>> -		hash__kernel_map_pages(page, numpages, enable);
>>> +		err =3D hash__kernel_map_pages(page, numpages, enable);
>>>   	else if (enable)
>>> -		set_memory_p(addr, numpages);
>>> +		err =3D set_memory_p(addr, numpages);
>>>   	else
>>> -		set_memory_np(addr, numpages);
>>> +		err =3D set_memory_np(addr, numpages);
>>> +
>>> +	if (err)
>>> +		panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");
>>=20
>> This doesn't compile, it's missing __func__ I guess.
>
> Damn, I was too quick when I took into account checkpatch's feedback,=20
> sorry for that.
>
>>=20
>> Seems like we could keep it simpler though, it should hopefully never
>> happen anyway, eg:
>>=20
>>    panic("%s: changing memory protections failed\n", __func__);
>
> Sure, let's do that. Do you want a v2 or you do the change directly ?

No need for a v2, I'll just fix it up when applying.

cheers