arch/powerpc/lib/code-patching.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
The only remaining use of map_patch_area() is mapping the zero page, and
immediately unmapping it again so that the intermediate page table
levels are all guaranteed to be populated.
The use of the zero page here is completely arbitrary, and not harmful
per se, but currently, it creates a writable mapping, and does so in a
manner that requires that the empty_zero_page[] symbol is not
const-qualified.
Given that this is about to change, and that map_patch_area() now never
maps anything other than the zero page, let's simplify the code and
- take the PA of empty_zero_page directly
- create a read-only temporary mapping.
This allows empty_zero_page[] to be repainted as const u8[] in a
subsequent patch, without making substantial changes to this code
patching logic.
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
Build tested only (Clang)
Resending from my non-Google email. Apologies if you are receiving this
twice.
arch/powerpc/lib/code-patching.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index f84e0337cc02..13a8acf851f1 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -60,7 +60,7 @@ struct patch_context {
static DEFINE_PER_CPU(struct patch_context, cpu_patching_context);
-static int map_patch_area(void *addr, unsigned long text_poke_addr);
+static int map_patch_area(unsigned long text_poke_addr);
static void unmap_patch_area(unsigned long addr);
static bool mm_patch_enabled(void)
@@ -117,7 +117,7 @@ static int text_area_cpu_up(unsigned int cpu)
// Map/unmap the area to ensure all page tables are pre-allocated
addr = (unsigned long)area->addr;
- err = map_patch_area(empty_zero_page, addr);
+ err = map_patch_area(addr);
if (err)
return err;
@@ -236,11 +236,10 @@ static unsigned long get_patch_pfn(void *addr)
/*
* This can be called for kernel text or a module.
*/
-static int map_patch_area(void *addr, unsigned long text_poke_addr)
+static int map_patch_area(unsigned long text_poke_addr)
{
- unsigned long pfn = get_patch_pfn(addr);
-
- return map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
+ return map_kernel_page(text_poke_addr, __pa_symbol(empty_zero_page),
+ PAGE_KERNEL_RO);
}
static void unmap_patch_area(unsigned long addr)
--
2.54.0.631.ge1b05301d1-goog
Le 20/05/2026 à 10:54, Ard Biesheuvel a écrit :
> The only remaining use of map_patch_area() is mapping the zero page, and
> immediately unmapping it again so that the intermediate page table
> levels are all guaranteed to be populated.
>
> The use of the zero page here is completely arbitrary, and not harmful
> per se, but currently, it creates a writable mapping, and does so in a
> manner that requires that the empty_zero_page[] symbol is not
> const-qualified.
>
> Given that this is about to change, and that map_patch_area() now never
> maps anything other than the zero page, let's simplify the code and
> - take the PA of empty_zero_page directly
> - create a read-only temporary mapping.
>
> This allows empty_zero_page[] to be repainted as const u8[] in a
> subsequent patch, without making substantial changes to this code
> patching logic.
>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> Build tested only (Clang)
>
> Resending from my non-Google email. Apologies if you are receiving this
> twice.
>
> arch/powerpc/lib/code-patching.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index f84e0337cc02..13a8acf851f1 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -60,7 +60,7 @@ struct patch_context {
>
> static DEFINE_PER_CPU(struct patch_context, cpu_patching_context);
>
> -static int map_patch_area(void *addr, unsigned long text_poke_addr);
> +static int map_patch_area(unsigned long text_poke_addr);
> static void unmap_patch_area(unsigned long addr);
>
> static bool mm_patch_enabled(void)
> @@ -117,7 +117,7 @@ static int text_area_cpu_up(unsigned int cpu)
>
> // Map/unmap the area to ensure all page tables are pre-allocated
> addr = (unsigned long)area->addr;
> - err = map_patch_area(empty_zero_page, addr);
> + err = map_patch_area(addr);
I would get rid of map_patch_area() completely and just do:
err = map_kernel_page(addr, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
By the way I don't know how vital it is to map in read-only but in case
it is see commit da3a3b0a0e38 ("powerpc/32s: map kasan zero shadow with
PAGE_READONLY instead of PAGE_KERNEL_RO")
> if (err)
> return err;
>
> @@ -236,11 +236,10 @@ static unsigned long get_patch_pfn(void *addr)
> /*
> * This can be called for kernel text or a module.
> */
> -static int map_patch_area(void *addr, unsigned long text_poke_addr)
> +static int map_patch_area(unsigned long text_poke_addr)
> {
> - unsigned long pfn = get_patch_pfn(addr);
> -
> - return map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
> + return map_kernel_page(text_poke_addr, __pa_symbol(empty_zero_page),
> + PAGE_KERNEL_RO);
> }
>
> static void unmap_patch_area(unsigned long addr)
On Wed, 20 May 2026, at 11:36, Christophe Leroy (CS GROUP) wrote:
> Le 20/05/2026 à 10:54, Ard Biesheuvel a écrit :
>> The only remaining use of map_patch_area() is mapping the zero page, and
>> immediately unmapping it again so that the intermediate page table
>> levels are all guaranteed to be populated.
>>
>> The use of the zero page here is completely arbitrary, and not harmful
>> per se, but currently, it creates a writable mapping, and does so in a
>> manner that requires that the empty_zero_page[] symbol is not
>> const-qualified.
>>
>> Given that this is about to change, and that map_patch_area() now never
>> maps anything other than the zero page, let's simplify the code and
>> - take the PA of empty_zero_page directly
>> - create a read-only temporary mapping.
>>
>> This allows empty_zero_page[] to be repainted as const u8[] in a
>> subsequent patch, without making substantial changes to this code
>> patching logic.
>>
>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>> Build tested only (Clang)
>>
>> Resending from my non-Google email. Apologies if you are receiving this
>> twice.
>>
>> arch/powerpc/lib/code-patching.c | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>> index f84e0337cc02..13a8acf851f1 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -60,7 +60,7 @@ struct patch_context {
>>
>> static DEFINE_PER_CPU(struct patch_context, cpu_patching_context);
>>
>> -static int map_patch_area(void *addr, unsigned long text_poke_addr);
>> +static int map_patch_area(unsigned long text_poke_addr);
>> static void unmap_patch_area(unsigned long addr);
>>
>> static bool mm_patch_enabled(void)
>> @@ -117,7 +117,7 @@ static int text_area_cpu_up(unsigned int cpu)
>>
>> // Map/unmap the area to ensure all page tables are pre-allocated
>> addr = (unsigned long)area->addr;
>> - err = map_patch_area(empty_zero_page, addr);
>> + err = map_patch_area(addr);
>
> I would get rid of map_patch_area() completely and just do:
>
> err = map_kernel_page(addr, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
>
I think retaining the symmetry of map_patch_area() and unmap_patch_area()
makes sense too.
>
> By the way I don't know how vital it is to map in read-only but in case
> it is see commit da3a3b0a0e38 ("powerpc/32s: map kasan zero shadow with
> PAGE_READONLY instead of PAGE_KERNEL_RO")
>
In the end, it doesn't make any difference, given that the mapping is removed
again immediately. It is just neater to use _RO to map a const object, rather
than explicitly creating a read-write mapping of something that should really
never be written to.
Le 20/05/2026 à 11:40, Ard Biesheuvel a écrit :
>
> On Wed, 20 May 2026, at 11:36, Christophe Leroy (CS GROUP) wrote:
>> Le 20/05/2026 à 10:54, Ard Biesheuvel a écrit :
>>> The only remaining use of map_patch_area() is mapping the zero page, and
>>> immediately unmapping it again so that the intermediate page table
>>> levels are all guaranteed to be populated.
>>>
>>> The use of the zero page here is completely arbitrary, and not harmful
>>> per se, but currently, it creates a writable mapping, and does so in a
>>> manner that requires that the empty_zero_page[] symbol is not
>>> const-qualified.
>>>
>>> Given that this is about to change, and that map_patch_area() now never
>>> maps anything other than the zero page, let's simplify the code and
>>> - take the PA of empty_zero_page directly
>>> - create a read-only temporary mapping.
>>>
>>> This allows empty_zero_page[] to be repainted as const u8[] in a
>>> subsequent patch, without making substantial changes to this code
>>> patching logic.
>>>
>>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>> Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>> Build tested only (Clang)
>>>
>>> Resending from my non-Google email. Apologies if you are receiving this
>>> twice.
>>>
>>> arch/powerpc/lib/code-patching.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>>> index f84e0337cc02..13a8acf851f1 100644
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -60,7 +60,7 @@ struct patch_context {
>>>
>>> static DEFINE_PER_CPU(struct patch_context, cpu_patching_context);
>>>
>>> -static int map_patch_area(void *addr, unsigned long text_poke_addr);
>>> +static int map_patch_area(unsigned long text_poke_addr);
>>> static void unmap_patch_area(unsigned long addr);
>>>
>>> static bool mm_patch_enabled(void)
>>> @@ -117,7 +117,7 @@ static int text_area_cpu_up(unsigned int cpu)
>>>
>>> // Map/unmap the area to ensure all page tables are pre-allocated
>>> addr = (unsigned long)area->addr;
>>> - err = map_patch_area(empty_zero_page, addr);
>>> + err = map_patch_area(addr);
>>
>> I would get rid of map_patch_area() completely and just do:
>>
>> err = map_kernel_page(addr, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
>>
>
> I think retaining the symmetry of map_patch_area() and unmap_patch_area()
> makes sense too.
Could also drop unmap_patch_area() and use unmap_kernel_page() instead.
>
>>
>> By the way I don't know how vital it is to map in read-only but in case
>> it is see commit da3a3b0a0e38 ("powerpc/32s: map kasan zero shadow with
>> PAGE_READONLY instead of PAGE_KERNEL_RO")
>>
>
> In the end, it doesn't make any difference, given that the mapping is removed
> again immediately. It is just neater to use _RO to map a const object, rather
> than explicitly creating a read-write mapping of something that should really
> never be written to.
>
Ok
On Wed, 20 May 2026, at 11:59, Christophe Leroy (CS GROUP) wrote:
> Le 20/05/2026 à 11:40, Ard Biesheuvel a écrit :
>>
>> On Wed, 20 May 2026, at 11:36, Christophe Leroy (CS GROUP) wrote:
>>> Le 20/05/2026 à 10:54, Ard Biesheuvel a écrit :
>>>> The only remaining use of map_patch_area() is mapping the zero page, and
>>>> immediately unmapping it again so that the intermediate page table
>>>> levels are all guaranteed to be populated.
>>>>
>>>> The use of the zero page here is completely arbitrary, and not harmful
>>>> per se, but currently, it creates a writable mapping, and does so in a
>>>> manner that requires that the empty_zero_page[] symbol is not
>>>> const-qualified.
>>>>
>>>> Given that this is about to change, and that map_patch_area() now never
>>>> maps anything other than the zero page, let's simplify the code and
>>>> - take the PA of empty_zero_page directly
>>>> - create a read-only temporary mapping.
>>>>
>>>> This allows empty_zero_page[] to be repainted as const u8[] in a
>>>> subsequent patch, without making substantial changes to this code
>>>> patching logic.
>>>>
>>>> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
>>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>>> Cc: "Christophe Leroy (CS GROUP)" <chleroy@kernel.org>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>> Build tested only (Clang)
>>>>
>>>> Resending from my non-Google email. Apologies if you are receiving this
>>>> twice.
>>>>
>>>> arch/powerpc/lib/code-patching.c | 11 +++++------
>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>>>> index f84e0337cc02..13a8acf851f1 100644
>>>> --- a/arch/powerpc/lib/code-patching.c
>>>> +++ b/arch/powerpc/lib/code-patching.c
>>>> @@ -60,7 +60,7 @@ struct patch_context {
>>>>
>>>> static DEFINE_PER_CPU(struct patch_context, cpu_patching_context);
>>>>
>>>> -static int map_patch_area(void *addr, unsigned long text_poke_addr);
>>>> +static int map_patch_area(unsigned long text_poke_addr);
>>>> static void unmap_patch_area(unsigned long addr);
>>>>
>>>> static bool mm_patch_enabled(void)
>>>> @@ -117,7 +117,7 @@ static int text_area_cpu_up(unsigned int cpu)
>>>>
>>>> // Map/unmap the area to ensure all page tables are pre-allocated
>>>> addr = (unsigned long)area->addr;
>>>> - err = map_patch_area(empty_zero_page, addr);
>>>> + err = map_patch_area(addr);
>>>
>>> I would get rid of map_patch_area() completely and just do:
>>>
>>> err = map_kernel_page(addr, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
>>>
>>
>> I think retaining the symmetry of map_patch_area() and unmap_patch_area()
>> makes sense too.
>
> Could also drop unmap_patch_area() and use unmap_kernel_page() instead.
>
Good point. That way, we'll end up with
arch/powerpc/lib/code-patching.c | 52 ++--------------------------------------
1 file changed, 2 insertions(+), 50 deletions(-
I'll spin a v2 with those changes once everyone on cc has had the opportunity
to chime in.
On 20/5/2026 20:16, Ard Biesheuvel wrote:
>
> On Wed, 20 May 2026, at 11:59, Christophe Leroy (CS GROUP) wrote:
>> Le 20/05/2026 à 11:40, Ard Biesheuvel a écrit :
>>>
>>> On Wed, 20 May 2026, at 11:36, Christophe Leroy (CS GROUP) wrote:
>>>> Le 20/05/2026 à 10:54, Ard Biesheuvel a écrit :
...
>>>>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>>>>> index f84e0337cc02..13a8acf851f1 100644
>>>>> --- a/arch/powerpc/lib/code-patching.c
>>>>> +++ b/arch/powerpc/lib/code-patching.c
>>>>> @@ -60,7 +60,7 @@ struct patch_context {
>>>>>
>>>>> static DEFINE_PER_CPU(struct patch_context, cpu_patching_context);
>>>>>
>>>>> -static int map_patch_area(void *addr, unsigned long text_poke_addr);
>>>>> +static int map_patch_area(unsigned long text_poke_addr);
>>>>> static void unmap_patch_area(unsigned long addr);
>>>>>
>>>>> static bool mm_patch_enabled(void)
>>>>> @@ -117,7 +117,7 @@ static int text_area_cpu_up(unsigned int cpu)
>>>>>
>>>>> // Map/unmap the area to ensure all page tables are pre-allocated
>>>>> addr = (unsigned long)area->addr;
>>>>> - err = map_patch_area(empty_zero_page, addr);
>>>>> + err = map_patch_area(addr);
>>>>
>>>> I would get rid of map_patch_area() completely and just do:
>>>>
>>>> err = map_kernel_page(addr, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
>>>>
>>>
>>> I think retaining the symmetry of map_patch_area() and unmap_patch_area()
>>> makes sense too.
>>
>> Could also drop unmap_patch_area() and use unmap_kernel_page() instead.
>>
>
> Good point. That way, we'll end up with
>
> arch/powerpc/lib/code-patching.c | 52 ++--------------------------------------
> 1 file changed, 2 insertions(+), 50 deletions(-
>
> I'll spin a v2 with those changes once everyone on cc has had the opportunity
> to chime in.
That diffstat is definitely attractive.
I do like that unmap_patch_area() is more defensive with the page table
walk, but it's probably overly paranoid. If page table levels have
vanished since we just mapped them then the system is probably toast anyway.
So OK by me.
cheers
© 2016 - 2026 Red Hat, Inc.