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

[Yang Shi <yang@os.amperecomputing.com>]

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