Re: [v3 PATCH 0/6] arm64: support FEAT_BBM level 2 and large block mapping when rodata=full

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

On 30/05/2025 18:18, Yang Shi wrote:
> 
> 
> On 5/30/25 12:59 AM, Ryan Roberts wrote:
>> On 29/05/2025 21:52, Yang Shi wrote:
>>>>>>>>> The split_mapping() guarantees keep block mapping if it is fully
>>>>>>>>> contained in
>>>>>>>>> the range between start and end, this is my series's responsibility. I
>>>>>>>>> know
>>>>>>>>> the
>>>>>>>>> current code calls apply_to_page_range() to apply permission change and it
>>>>>>>>> just
>>>>>>>>> does it on PTE basis. So IIUC Dev's series will modify it or provide a new
>>>>>>>>> API,
>>>>>>>>> then __change_memory_common() will call it to change permission. There
>>>>>>>>> should be
>>>>>>>>> some overlap between mine and Dev's, but I don't see strong dependency.
>>>>>>>> But if you have a block mapping in the region you are calling
>>>>>>>> __change_memory_common() on, today that will fail because it can only
>>>>>>>> handle
>>>>>>>> page mappings.
>>>>>>> IMHO letting __change_memory_common() manipulate on contiguous address
>>>>>>> range is
>>>>>>> another story and should be not a part of the split primitive.
>>>>>> I 100% agree that it should not be part of the split primitive.
>>>>>>
>>>>>> But your series *depends* upon __change_memory_common() being able to change
>>>>>> permissions on block mappings. Today it can only change permissions on page
>>>>>> mappings.
>>>>> I don't think split primitive depends on it. Changing permission on block
>>>>> mappings is just the user of the new split primitive IMHO. We just have no
>>>>> real
>>>>> user right now.
>>>> But your series introduces a real user; after your series, the linear map is
>>>> block mapped.
>>> The users of the split primitive are the permission changers, for example,
>>> module, bpf, secret mem, etc.
>> Ahh, perhaps this is the crux of our misunderstanding... In my model, the split
>> primitive is called from __change_memory_common() (or from other appropriate
>> functions in pageattr.c). It's an implementation detail for arm64 and is not
>> exposed to common code. arm64 knows that it can split live mappings in a
>> transparent way so it uses huge pages eagerly and splits on demand.
>>
>> I personally wouldn't want to be relying on the memory user knowing it needs to
>> split the mappings...
> 
> We are actually on the same page...
> 
> For example, when loading module, kernel currently does:
> 
> vmalloc() // Allocate memory for module
> module_enable_text_rox() // change permission to ROX for text section
>     set_memory_x
>         change_memory_common
>             for every page in the vmalloc area
>                 __change_memory_common(addr, PAGE_SIZE, ...) // page basis
>                     split_mapping(addr, addr + PAGE_SIZE)
>                     apply_to_page_range() // apply the new permission
> 
> __change_memory_common() has to be called on page basis because we don't know
> whether the pages for the vmalloc area are contiguous or not. So the split
> primitive is called on page basis.

Yes that makes sense for the case where we are setting permissions on a
virtually contiguous region of vmalloc space; in that case we must set
permissions on the linear map page-by-page. Agreed.

I was thinking of the cases where we are changing the permissions on a virtually
contiguous region of the *linear map*. Although looking again at the code, it
seems there aren't as many places as I thought that actually do this. I think
set_direct_map_valid_noflush() is the only one that will operate on multiple
pages of the linear map at a time. But this single case means that you could end
up wanting to change permissions on a large block mapping and therefore need
Dev's work, right?

Thanks,
Ryan

> 
> 
> So we need do the below in order to keep large mapping:
> check whether the vmalloc area is huge mapped (PMD/CONT PMD/CONT PTE) or not
> if (it is huge mapped)
>     __change_memory_common(addr, HUGE_SIZE, ...)
>         split_mapping(addr, addr + HUGE_SIZE)
>         change permission on (addr, addr + HUGE_SIZE)
> else
>     fallback to page basis
> 
> 
> To have huge mapping for vmalloc, we need use vmalloc_huge() or the new
> implementation proposed by you to allocate memory for module in the first place.
> This is the "user" in my understanding.
> 
> Thanks,
> Yang
> 
>