The kprobe page is allocated by execmem allocator with ROX permission.
It needs to call set_memory_rox() to set proper permission for the
direct map too. It was missed.

And the set_memory_rox() guarantees the direct map will be split if it
needs so that set_direct_map calls in vfree() won't fail.

Fixes: 10d5e97c1bf8 ("arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page")
Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
---
 arch/arm64/kernel/probes/kprobes.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 0c5d408afd95..c4f8c4750f1e 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -10,6 +10,7 @@
 
 #define pr_fmt(fmt) "kprobes: " fmt
 
+#include <linux/execmem.h>
 #include <linux/extable.h>
 #include <linux/kasan.h>
 #include <linux/kernel.h>
@@ -41,6 +42,17 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 static void __kprobes
 post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
 
+void *alloc_insn_page(void)
+{
+	void *page;
+
+	page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
+	if (!page)
+		return NULL;
+	set_memory_rox((unsigned long)page, 1);
+	return page;
+}
+
 static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 {
 	kprobe_opcode_t *addr = p->ainsn.xol_insn;
-- 
2.47.0

On Wed, Sep 17, 2025 at 12:02:11PM -0700, Yang Shi wrote:
> The kprobe page is allocated by execmem allocator with ROX permission.
> It needs to call set_memory_rox() to set proper permission for the
> direct map too. It was missed.
> 
> And the set_memory_rox() guarantees the direct map will be split if it
> needs so that set_direct_map calls in vfree() won't fail.
> 
> Fixes: 10d5e97c1bf8 ("arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page")
> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
> ---
>  arch/arm64/kernel/probes/kprobes.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 0c5d408afd95..c4f8c4750f1e 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -10,6 +10,7 @@
>  
>  #define pr_fmt(fmt) "kprobes: " fmt
>  
> +#include <linux/execmem.h>
>  #include <linux/extable.h>
>  #include <linux/kasan.h>
>  #include <linux/kernel.h>
> @@ -41,6 +42,17 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  static void __kprobes
>  post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
>  
> +void *alloc_insn_page(void)
> +{
> +	void *page;

Nit: I'd call this 'addr'. 'page' makes me think of a struct page.

> +
> +	page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
> +	if (!page)
> +		return NULL;
> +	set_memory_rox((unsigned long)page, 1);

It's unfortunate that we change the attributes of the ROX vmap first to
RO, then to back to ROX so that we get the linear map changed. Maybe
factor out some of the code in change_memory_common() to only change the
linear map.

Otherwise it looks fine.

-- 
Catalin

On 9/18/25 5:48 AM, Catalin Marinas wrote:
> On Wed, Sep 17, 2025 at 12:02:11PM -0700, Yang Shi wrote:
>> The kprobe page is allocated by execmem allocator with ROX permission.
>> It needs to call set_memory_rox() to set proper permission for the
>> direct map too. It was missed.
>>
>> And the set_memory_rox() guarantees the direct map will be split if it
>> needs so that set_direct_map calls in vfree() won't fail.
>>
>> Fixes: 10d5e97c1bf8 ("arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page")
>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>> ---
>>   arch/arm64/kernel/probes/kprobes.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
>> index 0c5d408afd95..c4f8c4750f1e 100644
>> --- a/arch/arm64/kernel/probes/kprobes.c
>> +++ b/arch/arm64/kernel/probes/kprobes.c
>> @@ -10,6 +10,7 @@
>>   
>>   #define pr_fmt(fmt) "kprobes: " fmt
>>   
>> +#include <linux/execmem.h>
>>   #include <linux/extable.h>
>>   #include <linux/kasan.h>
>>   #include <linux/kernel.h>
>> @@ -41,6 +42,17 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>>   static void __kprobes
>>   post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
>>   
>> +void *alloc_insn_page(void)
>> +{
>> +	void *page;
> Nit: I'd call this 'addr'. 'page' makes me think of a struct page.

Sure.

>
>> +
>> +	page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
>> +	if (!page)
>> +		return NULL;
>> +	set_memory_rox((unsigned long)page, 1);
> It's unfortunate that we change the attributes of the ROX vmap first to
> RO, then to back to ROX so that we get the linear map changed. Maybe
> factor out some of the code in change_memory_common() to only change the
> linear map.

I want to make sure I understand you correctly, you meant 
set_memory_rox() should do:

change linear map to RO (call a new helper, for example, 
set_direct_map_ro())
change vmap to ROX (call change_memory_common())

Is it correct?

If so set_memory_ro() should do the similar thing.

And I think we should have the cleanup patch separate from this bug fix 
patch because the bug fix patch should be applied to -stable release 
too. Keeping it simpler makes the backport easier.

Shall I squash the cleanup patch into patch #1?

Thanks,
Yang

>
> Otherwise it looks fine.
>

On Thu, Sep 18, 2025 at 08:05:55AM -0700, Yang Shi wrote:
> On 9/18/25 5:48 AM, Catalin Marinas wrote:
> > On Wed, Sep 17, 2025 at 12:02:11PM -0700, Yang Shi wrote:
> > > +	page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
> > > +	if (!page)
> > > +		return NULL;
> > > +	set_memory_rox((unsigned long)page, 1);
> > It's unfortunate that we change the attributes of the ROX vmap first to
> > RO, then to back to ROX so that we get the linear map changed. Maybe
> > factor out some of the code in change_memory_common() to only change the
> > linear map.
> 
> I want to make sure I understand you correctly, you meant set_memory_rox()
> should do:
> 
> change linear map to RO (call a new helper, for example,
> set_direct_map_ro())
> change vmap to ROX (call change_memory_common())

set_memory_rox() is correct. What I meant is that in alloc_insn_page(),
execmem_alloc() already returns RX memory. Calling set_memory_rox() does
indeed change the linear map to RO but it also changes the vmap memory
to RO and then to RX. There's no need for the alloc_insn_page() to do
this but we shouldn't change set_memory_rox() for this, the latter is
correct. I was thinking of alloc_insn_page() calling a new function that
only changes the linear map.

> And I think we should have the cleanup patch separate from this bug fix
> patch because the bug fix patch should be applied to -stable release too.
> Keeping it simpler makes the backport easier.

Yes, for now you can leave it as is, that's not a critical path.

> Shall I squash the cleanup patch into patch #1?

No, I'd leave it as a separate fix, especially if we want to backport
it.

Anyway, for now, with the nitpick on the address variable name:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

On 9/18/25 8:32 AM, Catalin Marinas wrote:
> On Thu, Sep 18, 2025 at 08:05:55AM -0700, Yang Shi wrote:
>> On 9/18/25 5:48 AM, Catalin Marinas wrote:
>>> On Wed, Sep 17, 2025 at 12:02:11PM -0700, Yang Shi wrote:
>>>> +	page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
>>>> +	if (!page)
>>>> +		return NULL;
>>>> +	set_memory_rox((unsigned long)page, 1);
>>> It's unfortunate that we change the attributes of the ROX vmap first to
>>> RO, then to back to ROX so that we get the linear map changed. Maybe
>>> factor out some of the code in change_memory_common() to only change the
>>> linear map.
>> I want to make sure I understand you correctly, you meant set_memory_rox()
>> should do:
>>
>> change linear map to RO (call a new helper, for example,
>> set_direct_map_ro())
>> change vmap to ROX (call change_memory_common())
> set_memory_rox() is correct. What I meant is that in alloc_insn_page(),
> execmem_alloc() already returns RX memory. Calling set_memory_rox() does
> indeed change the linear map to RO but it also changes the vmap memory
> to RO and then to RX. There's no need for the alloc_insn_page() to do
> this but we shouldn't change set_memory_rox() for this, the latter is
> correct. I was thinking of alloc_insn_page() calling a new function that
> only changes the linear map.

Aha, I see. If we have the new helper, it also allows us to refactor 
set_memory_rox() to what I said.

>
>> And I think we should have the cleanup patch separate from this bug fix
>> patch because the bug fix patch should be applied to -stable release too.
>> Keeping it simpler makes the backport easier.
> Yes, for now you can leave it as is, that's not a critical path.

Sure.

>
>> Shall I squash the cleanup patch into patch #1?
> No, I'd leave it as a separate fix, especially if we want to backport
> it.

I meant the potential cleanup patch. Anyway it can be handled in a 
separate patch at anytime.

>
> Anyway, for now, with the nitpick on the address variable name:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thank you. Ryan also suggested separate the fix from this series. I will 
fix the variable name nit then post it separately instead of posting a 
new series.

Yang

On 18/09/2025 16:05, Yang Shi wrote:
> 
> 
> On 9/18/25 5:48 AM, Catalin Marinas wrote:
>> On Wed, Sep 17, 2025 at 12:02:11PM -0700, Yang Shi wrote:
>>> The kprobe page is allocated by execmem allocator with ROX permission.
>>> It needs to call set_memory_rox() to set proper permission for the
>>> direct map too. It was missed.
>>>
>>> And the set_memory_rox() guarantees the direct map will be split if it
>>> needs so that set_direct_map calls in vfree() won't fail.
>>>
>>> Fixes: 10d5e97c1bf8 ("arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page")
>>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kernel/probes/kprobes.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/
>>> kprobes.c
>>> index 0c5d408afd95..c4f8c4750f1e 100644
>>> --- a/arch/arm64/kernel/probes/kprobes.c
>>> +++ b/arch/arm64/kernel/probes/kprobes.c
>>> @@ -10,6 +10,7 @@
>>>     #define pr_fmt(fmt) "kprobes: " fmt
>>>   +#include <linux/execmem.h>
>>>   #include <linux/extable.h>
>>>   #include <linux/kasan.h>
>>>   #include <linux/kernel.h>
>>> @@ -41,6 +42,17 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>>>   static void __kprobes
>>>   post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs
>>> *);
>>>   +void *alloc_insn_page(void)
>>> +{
>>> +    void *page;
>> Nit: I'd call this 'addr'. 'page' makes me think of a struct page.
> 
> Sure.
> 
>>
>>> +
>>> +    page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
>>> +    if (!page)
>>> +        return NULL;
>>> +    set_memory_rox((unsigned long)page, 1);
>> It's unfortunate that we change the attributes of the ROX vmap first to
>> RO, then to back to ROX so that we get the linear map changed. Maybe
>> factor out some of the code in change_memory_common() to only change the
>> linear map.
> 
> I want to make sure I understand you correctly, you meant set_memory_rox()
> should do:
> 
> change linear map to RO (call a new helper, for example, set_direct_map_ro())
> change vmap to ROX (call change_memory_common())
> 
> Is it correct?
> 
> If so set_memory_ro() should do the similar thing.
> 
> And I think we should have the cleanup patch separate from this bug fix patch
> because the bug fix patch should be applied to -stable release too. Keeping it
> simpler makes the backport easier.
> 
> Shall I squash the cleanup patch into patch #1?


Personally I think we should drop this patch from the series and handle it
separately.

We worked out that the requirement is to either never call set_memory_*() or to
call set_memory_*() for the entire vmalloc'ed range prior to optionally calling
set_memory_*() for a sub-range in order to guarrantee vm_reset_perms() works
correctly.

Given this is only allocating a single page, it is impossible to call
set_memory_*() for a sub-range. So the requirement is met.

I agree it looks odd/wrong to have different permissions in the linear map vs
the vmap but that is an orthogonal bug that can be fixed separately.

What do you think?

Thanks,
Ryan


> 
> Thanks,
> Yang
> 
>>
>> Otherwise it looks fine.
>>
> 

On 9/18/25 8:30 AM, Ryan Roberts wrote:
> On 18/09/2025 16:05, Yang Shi wrote:
>>
>> On 9/18/25 5:48 AM, Catalin Marinas wrote:
>>> On Wed, Sep 17, 2025 at 12:02:11PM -0700, Yang Shi wrote:
>>>> The kprobe page is allocated by execmem allocator with ROX permission.
>>>> It needs to call set_memory_rox() to set proper permission for the
>>>> direct map too. It was missed.
>>>>
>>>> And the set_memory_rox() guarantees the direct map will be split if it
>>>> needs so that set_direct_map calls in vfree() won't fail.
>>>>
>>>> Fixes: 10d5e97c1bf8 ("arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page")
>>>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>>>> ---
>>>>    arch/arm64/kernel/probes/kprobes.c | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/
>>>> kprobes.c
>>>> index 0c5d408afd95..c4f8c4750f1e 100644
>>>> --- a/arch/arm64/kernel/probes/kprobes.c
>>>> +++ b/arch/arm64/kernel/probes/kprobes.c
>>>> @@ -10,6 +10,7 @@
>>>>      #define pr_fmt(fmt) "kprobes: " fmt
>>>>    +#include <linux/execmem.h>
>>>>    #include <linux/extable.h>
>>>>    #include <linux/kasan.h>
>>>>    #include <linux/kernel.h>
>>>> @@ -41,6 +42,17 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>>>>    static void __kprobes
>>>>    post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs
>>>> *);
>>>>    +void *alloc_insn_page(void)
>>>> +{
>>>> +    void *page;
>>> Nit: I'd call this 'addr'. 'page' makes me think of a struct page.
>> Sure.
>>
>>>> +
>>>> +    page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
>>>> +    if (!page)
>>>> +        return NULL;
>>>> +    set_memory_rox((unsigned long)page, 1);
>>> It's unfortunate that we change the attributes of the ROX vmap first to
>>> RO, then to back to ROX so that we get the linear map changed. Maybe
>>> factor out some of the code in change_memory_common() to only change the
>>> linear map.
>> I want to make sure I understand you correctly, you meant set_memory_rox()
>> should do:
>>
>> change linear map to RO (call a new helper, for example, set_direct_map_ro())
>> change vmap to ROX (call change_memory_common())
>>
>> Is it correct?
>>
>> If so set_memory_ro() should do the similar thing.
>>
>> And I think we should have the cleanup patch separate from this bug fix patch
>> because the bug fix patch should be applied to -stable release too. Keeping it
>> simpler makes the backport easier.
>>
>> Shall I squash the cleanup patch into patch #1?
>
> Personally I think we should drop this patch from the series and handle it
> separately.
>
> We worked out that the requirement is to either never call set_memory_*() or to
> call set_memory_*() for the entire vmalloc'ed range prior to optionally calling
> set_memory_*() for a sub-range in order to guarrantee vm_reset_perms() works
> correctly.
>
> Given this is only allocating a single page, it is impossible to call
> set_memory_*() for a sub-range. So the requirement is met.
>
> I agree it looks odd/wrong to have different permissions in the linear map vs
> the vmap but that is an orthogonal bug that can be fixed separately.
>
> What do you think?

Yeah, sounds good to me.

Thanks,
Yang

>
> Thanks,
> Ryan
>
>
>> Thanks,
>> Yang
>>
>>> Otherwise it looks fine.
>>>