Re: [PATCH] arm64/pageattr: Propagate return value from __change_memory_common

[Dev Jain]

On 05/11/25 9:27 am, Dev Jain wrote:
> On 04/11/25 6:26 pm, Will Deacon wrote:
> > On Tue, Nov 04, 2025 at 09:06:12AM +0530, Dev Jain wrote:
> > > On 04/11/25 12:15 am, Yang Shi wrote:
> > > > On 11/3/25 7:16 AM, Will Deacon wrote:
> > > > > On Mon, Nov 03, 2025 at 11:43:06AM +0530, Dev Jain wrote:
> > > > > > Post a166563e7ec3 ("arm64: mm: support large block mapping when
> > > > > > rodata=full"),
> > > > > > __change_memory_common has a real chance of failing due to split
> > > > > > failure.
> > > > > > Before that commit, this line was introduced in c55191e96caa,
> > > > > > still having
> > > > > > a chance of failing if it needs to allocate pagetable memory in
> > > > > > apply_to_page_range, although that has never been observed to be 
> > > > > > true.
> > > > > > In general, we should always propagate the return value to the 
> > > > > > caller.
> > > > > >
> > > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > > Fixes: c55191e96caa ("arm64: mm: apply r/o permissions of VM
> > > > > > areas to its linear alias as well")
> > > > > > Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> > > > > > ---
> > > > > > Based on Linux 6.18-rc4.
> > > > > >
> > > > > >    arch/arm64/mm/pageattr.c | 5 ++++-
> > > > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> > > > > > index 5135f2d66958..b4ea86cd3a71 100644
> > > > > > --- a/arch/arm64/mm/pageattr.c
> > > > > > +++ b/arch/arm64/mm/pageattr.c
> > > > > > @@ -148,6 +148,7 @@ static int change_memory_common(unsigned
> > > > > > long addr, int numpages,
> > > > > >        unsigned long size = PAGE_SIZE * numpages;
> > > > > >        unsigned long end = start + size;
> > > > > >        struct vm_struct *area;
> > > > > > +    int ret;
> > > > > >        int i;
> > > > > >          if (!PAGE_ALIGNED(addr)) {
> > > > > > @@ -185,8 +186,10 @@ static int change_memory_common(unsigned
> > > > > > long addr, int numpages,
> > > > > >        if (rodata_full && (pgprot_val(set_mask) == PTE_RDONLY ||
> > > > > >                    pgprot_val(clear_mask) == PTE_RDONLY)) {
> > > > > >            for (i = 0; i < area->nr_pages; i++) {
> > > > > > - __change_memory_common((u64)page_address(area->pages[i]),
> > > > > > +            ret =
> > > > > > __change_memory_common((u64)page_address(area->pages[i]),
> > > > > >                               PAGE_SIZE, set_mask, clear_mask);
> > > > > > +            if (ret)
> > > > > > +                return ret;
> > > > > Hmm, this means we can return failure half-way through the 
> > > > > operation. Is
> > > > > that something callers are expecting to handle? If so, how can 
> > > > > they tell
> > > > > how far we got?
> > > > IIUC the callers don't have to know whether it is half-way or not
> > > > because the callers will change the permission back (e.g. to RW) 
> > > > for the
> > > > whole range when freeing memory.
> > > Yes, it is the caller's responsibility to set VM_FLUSH_RESET_PERMS 
> > > flag.
> > > Upon vfree(), it will change the direct map permissions back to RW.
> > Ok, but vfree() ends up using update_range_prot() to do that and if we
> > need to worry about that failing (as per your commit message), then
> > we're in trouble because the calls to set_area_direct_map() are 
> > unchecked.
> >
> > In other words, this patch is either not necessary or it is incomplete.
>
> Here is the relevant email, in the discussion between Ryan and Yang:
>
> https://lore.kernel.org/all/fe52a1d8-5211-4962-afc8-c3f9caf64119@xxxxxxxxxxxxxxxxxxxxxx/ 
>
>
> We had concluded that all callers of set_memory_ro() or 
> set_memory_rox() (which require the
> linear map perm change back to default, upon vfree() ) will call it 
> for the entire region (vm_struct).
> So, when we do the set_direct_map_invalid_noflush, it is guaranteed 
> that the region has already
> been split. So this call cannot fail.
>
> https://lore.kernel.org/all/f8898c87-8f49-4ef2-86ae-b60bcf67658c@xxxxxxxxxxxxxxxxxxxxxx/ 
>
>
> This email notes that there is some code doing set_memory_rw() and 
> unnecessarily setting the VM_FLUSH_RESET_PERMS
> flag, but in that case we don't care about the 
> set_direct_map_invalid_noflush call failing because the protections
> are already RW.
>
> Although we had also observed that all of this is fragile and depends 
> on the caller doing the
> correct thing. The real solution should be somehow getting rid of the 
> BBM style invalidation.
> Ryan had proposed some methods in that email thread.
>
> One solution which I had thought of, is that, observe that we are 
> doing an overkill by
> setting the linear map to invalid and then default, for the *entire* 
> region. What we
> can do is iterate over the linear map alias of the vm_struct *area and 
> only change permission
> back to RW for the pages which are *not* RW. And, those relevant 
> mappings are guaranteed to
> be split because they were changed from RW to not RW.

@Yang and Ryan,

I saw Yang's patch here:
https://lore.kernel.org/all/20251023204428.477531-1-yang@xxxxxxxxxxxxxxxxxxxxxx/
and realized that currently we are splitting away the linear map alias 
of the *entire* region.

Shouldn't this then imply that set_direct_map_invalid_noflush will never 
fail, since even

a set_memory_rox() call on a single page will split the linear map for 
the entire region,

and thus there is no fragility here which we were discussing about? I 
may be forgetting

something, this linear map stuff is confusing enough already.


> > Will