Re: [PATCH v8 5/5] arm64: kprobes: call set_memory_rox() for kprobe page

[Ryan Roberts <ryan.roberts@arm.com>]

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.
>>
>