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