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

From: Yang Shi

Date: Tue Sep 09 2025 - 13:32:26 EST



On 9/9/25 9:32 AM, Ryan Roberts wrote:
On 09/09/2025 16:32, Yang Shi wrote:

On 9/9/25 7:36 AM, Ryan Roberts wrote:
On 08/09/2025 19:31, Yang Shi wrote:
On 9/8/25 9:34 AM, Ryan Roberts wrote:
On 04/09/2025 22:49, Yang Shi wrote:
On 9/4/25 10:47 AM, Yang Shi wrote:
On 9/4/25 6:16 AM, Ryan Roberts wrote:
On 04/09/2025 14:14, Ryan Roberts wrote:
On 03/09/2025 01:50, Yang Shi wrote:
I am wondering whether we can just have a warn_on_once or something
for the
case
when we fail to allocate a pagetable page. Or, Ryan had
suggested in an off-the-list conversation that we can maintain a cache
of PTE
tables for every PMD block mapping, which will give us
the same memory consumption as we do today, but not sure if this is
worth it.
x86 can already handle splitting but due to the callchains
I have described above, it has the same problem, and the code has been
working
for years :)
I think it's preferable to avoid having to keep a cache of pgtable
memory
if we
can...
Yes, I agree. We simply don't know how many pages we need to cache,
and it
still can't guarantee 100% allocation success.
This is wrong... We can know how many pages will be needed for splitting
linear
mapping to PTEs for the worst case once linear mapping is finalized.
But it
may
require a few hundred megabytes memory to guarantee allocation success. I
don't
think it is worth for such rare corner case.
Indeed, we know exactly how much memory we need for pgtables to map the
linear
map by pte - that's exactly what we are doing today. So we _could_ keep a
cache.
We would still get the benefit of improved performance but we would lose
the
benefit of reduced memory.

I think we need to solve the vm_reset_perms() problem somehow, before we
can
enable this.
Sorry I realise this was not very clear... I am saying I think we need to
fix it
somehow. A cache would likely work. But I'd prefer to avoid it if we can
find a
better solution.
Took a deeper look at vm_reset_perms(). It was introduced by commit
868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions").
The
VM_FLUSH_RESET_PERMS flag is supposed to be set if the vmalloc memory is RO
and/or ROX. So set_memory_ro() or set_memory_rox() is supposed to follow up
vmalloc(). So the page table should be already split before reaching vfree().
I think this why vm_reset_perms() doesn't not check return value.
If vm_reset_perms() is assuming it can't/won't fail, I think it should at least
output a warning if it does?
It should. Anyway warning will be raised if split fails. We have somehow
mitigation.

I scrutinized all the callsites with VM_FLUSH_RESET_PERMS flag set.
Just checking; I think you made a comment before about there only being a few
sites that set VM_FLUSH_RESET_PERMS. But one of them is the helper,
set_vm_flush_reset_perms(). So just making sure you also followed to the places
that use that helper?
Yes, I did.

The most
of them has set_memory_ro() or set_memory_rox() followed.
And are all callsites calling set_memory_*() for the entire cell that was
allocated by vmalloc? If there are cases where it only calls that for a portion
of it, then it's not gurranteed that the memory is correctly split.
Yes, all callsites call set_memory_*() for the entire range.

But there are 3
places I don't see set_memory_ro()/set_memory_rox() is called.

1. BPF trampoline allocation. The BPF trampoline calls
arch_protect_bpf_trampoline(). The generic implementation does call
set_memory_rox(). But the x86 and arm64 implementation just simply return 0.
For x86, it is because execmem cache is used and it does call
set_memory_rox(). ARM64 doesn't need to split page table before this series,
so it should never fail. I think we just need to use the generic
implementation (remove arm64 implementation) if this series is merged.
I know zero about BPF. But it looks like the allocation happens in
arch_alloc_bpf_trampoline(), which for arm64, calls bpf_prog_pack_alloc(). And
for small sizes, it grabs some memory from a "pack". So doesn't this mean that
you are calling set_memory_rox() for a sub-region of the cell, so that doesn't
actually help at vm_reset_perms()-time?
Took a deeper look at bpf pack allocator. The "pack" is allocated by
alloc_new_pack(), which does:
bpf_jit_alloc_exec()
set_vm_flush_reset_perms()
set_memory_rox()

If the size is greater than the pack size, it calls:
bpf_jit_alloc_exec()
set_vm_flush_reset_perms()
set_memory_rox()

So it looks like bpf trampoline is good, and we don't need do anything. It
should be removed from the list. I didn't look deep enough for bpf pack
allocator in the first place.

2. BPF dispatcher. It calls execmem_alloc which has VM_FLUSH_RESET_PERMS set.
But it is used for rw allocation, so VM_FLUSH_RESET_PERMS should be
unnecessary IIUC. So it doesn't matter even though vm_reset_perms() fails.

3. kprobe. S390's alloc_insn_page() does call set_memory_rox(), x86 also
called set_memory_rox() before switching to execmem cache. The execmem cache
calls set_memory_rox(). I don't know why ARM64 doesn't call it.

So I think we just need to fix #1 and #3 per the above analysis. If this
analysis look correct to you guys, I will prepare two patches to fix them.
This all seems quite fragile. I find it interesting that vm_reset_perms() is
doing break-before-make; it sets the PTEs as invalid, then flushes the TLB,
then
sets them to default. But for arm64, at least, I think break-before-make is not
required. We are only changing the permissions so that can be done on live
mappings; essentially change the sequence to; set default, flush TLB.
Yeah, I agree it is a little bit fragile. I think this is the "contract" for
vmalloc users. You allocate ROX memory via vmalloc, you are required to call
set_memory_*(). But there is nothing to guarantee the "contract" is followed.
But I don't think this is the only case in kernel.

If we do that, then if the memory was already default, then there is no need to
do anything (so no chance of allocation failure). If the memory was not
default,
then it must have already been split to make it non-default, in which case we
can also gurrantee that no allocations are required.

What am I missing?
The comment says:
Set direct map to something invalid so that it won't be cached if there are any
accesses after the TLB flush, then flush the TLB and reset the direct map
permissions to the default.

IIUC, it guarantees the direct map can't be cached in TLB after TLB flush from
_vm_unmap_aliases() by setting them invalid because TLB never cache invalid
entries. Skipping set direct map to invalid seems break this. Or "changing
permission on live mappings" on ARM64 can achieve the same goal?
Here's my understanding of the intent of the code:

Let's say we start with some memory that has been mapped RO. Our goal is to
reset the memory back to RW and ensure that no TLB entry remains in the TLB for
the old RO mapping. There are 2 ways to do that:


Approach 1 (used in current code):
1. set PTE to invalid
2. invalidate any TLB entry for the VA
3. set the PTE to RW

Approach 2:
1. set the PTE to RW
2. invalidate any TLB entry for the VA
IIUC, the intent of the code is "reset direct map permission *without* leaving a
RW+X window". The TLB flush call actually flushes both VA and direct map together.
So if this is the intent, approach #2 may have VA with X permission but direct
map may be RW at the mean time. It seems break the intent.
Ahh! Thanks, it's starting to make more sense now.

Though on first sight it seems a bit mad to me to form a tlb flush range that
covers all the direct map pages and all the lazy vunmap regions. Is that
intended to be a perf optimization or something else? It's not clear from the
history.

I think it should be mainly performance driven. I can't see how come two TLB flushes (for vmap and direct map respectively) don't work if I don't miss something.



Could this be split into 2 operations?

1. unmap the aliases (+ tlbi the aliases).
2. set the direct memory back to default (+ tlbi the direct map region).

The only 2 potential problems I can think of are;

- Performance: 2 tlbis instead of 1, but conversely we probably avoid flushing
a load of TLB entries that we didn't really need to.

The two tlbis should work. But performance is definitely a concern. It may be hard to justify how much performance impact caused by over flush, but multiple TLBIs is definitely not preferred, particularly on some large scale machines. We have experienced some scalability issues with TLBI due to the large core count on Ampere systems.

- Given there is now no lock around the tlbis (currently it's under
vmap_purge_lock) is there a race where a new alias can appear between steps 1
and 2? I don't think so, because the memory is allocated to the current mapping
so how is it going to get re-mapped?

Yes, I agree. I don't think the race is real. The physical pages will not be freed until vm_reset_perms() is done. The VA may be reallocated, but it will be mapped to different physical pages.



Could this solve it?

I think it could. But the potential performance impact (two TLBIs) is a real concern.

Anyway the vmalloc user should call set_memory_*() for any RO/ROX mapping, set_memory_*() should split the page table before reaching vm_reset_perms() so it should not fail. If set_memory_*() is not called, it is a bug, it should be fixed, like ARM64 kprobes.

It is definitely welcome to make it more robust, although the warning from split may mitigate this somehow. But I don't think this should be a blocker for this series IMHO.

Thanks,
Yang




Thanks,
Yang

The benefit of approach 1 is that it is guarranteed that it is impossible for
different CPUs to have different translations for the same VA in their
respective TLB. But for approach 2, it's possible that between steps 1 and 2, 1
CPU has a RO entry and another CPU has a RW entry. But that will get fixed once
the TLB is flushed - it's not really an issue.

(There is probably also an obscure way to end up with 2 TLB entries (one with RO
and one with RW) for the same CPU, but the arm64 architecture permits that as
long as it's only a permission mismatch).

Anyway, approach 2 is used when changing memory permissions on user mappings, so
I don't see why we can't take the same approach here. That would solve this
whole class of issue for us.

Thanks,
Ryan


Thanks,
Yang

Thanks,
Ryan


Tested the below patch with bpftrace kfunc (allocate bpf trampoline) and
kprobes. It seems work well.

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;
+
+       page = execmem_alloc(EXECMEM_KPROBES, PAGE_SIZE);
+       if (!page)
+               return NULL;
+       set_memory_rox((unsigned long)page, 1);
+       return page;
+}
+
   static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
   {
          kprobe_opcode_t *addr = p->ainsn.xol_insn;
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 52ffe115a8c4..3e301bc2cd66 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2717,11 +2717,6 @@ void arch_free_bpf_trampoline(void *image, unsigned int
size)
          bpf_prog_pack_free(image, size);
   }

-int arch_protect_bpf_trampoline(void *image, unsigned int size)
-{
-       return 0;
-}
-
   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
                                  void *ro_image_end, const struct
btf_func_model *m,
                                  u32 flags, struct bpf_tramp_links *tlinks,


Thanks,
Yang

Thanks,
Ryan

Thanks,
Yang

Thanks,
Yang

Thanks,
Ryan