[PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[Matthew Wood]

Widen the text_mutex critical section in bpf_arch_text_poke() to cover
the page permission changes made while updating a bpf prog's plt target.
When a long-jump trampoline is attached to or detached from a bpf prog,
the plt target at the end of the prog is currently updated before
text_mutex is taken, by temporarily making the page writable:

	set_memory_rw(page);
	WRITE_ONCE(plt->target, plt_target);
	set_memory_ro(page);

Leaving this outside text_mutex assumes the page belongs only to the
prog being poked, whose pokes are already serialized by its trampoline
lock. Commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory
management") broke that assumption; it switched the arm64 JIT to the bpf
prog pack allocator, which packs programs into small chunks of large
shared allocations, without updating bpf_arch_text_poke(). Since then,
unrelated progs (and their plts) commonly share a page. Two CPUs
attaching to or detaching from different target progs in the same page
hold different trampoline locks, so their permission changes can
interleave:

	CPU A                           CPU B
	set_memory_rw(page)
	                                set_memory_rw(page)
	                                WRITE_ONCE(plt_B->target, ...)
	                                set_memory_ro(page)
	WRITE_ONCE(plt_A->target, ...)  <- permission fault

This was hit on a 64K page arm64 machine:

  Unable to handle kernel write to read-only memory at virtual address ffff80008f46d5f8
  FSC = 0x0f: level 3 permission fault
  pte=00c00200ea5a0783
  pc : bpf_arch_text_poke+0x214/0x238
  Call trace:
   bpf_arch_text_poke+0x214/0x238 (P)
   __bpf_trampoline_link_prog+0x1c8/0x470
   bpf_trampoline_link_prog+0x64/0x90
   bpf_tracing_prog_attach+0x318/0x4a8
   bpf_raw_tp_link_attach+0x104/0x258
   bpf_raw_tracepoint_open+0x6c/0x90
   __sys_bpf+0x134c/0x3e10

Fixes: 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management")
Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
---
 arch/arm64/net/bpf_jit_comp.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 475e70653454..66fab0bfd3da 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -3338,12 +3338,20 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
 		 */
 		plt_target = (u64)&dummy_tramp;

+	/* pages of the bpf prog pack are shared between progs, so the
+	 * set_memory_rw()/set_memory_ro() window below must be serialized
+	 * against other pokers too.
+	 */
+	mutex_lock(&text_mutex);
+
 	if (plt_target) {
 		/* non-zero plt_target indicates we're patching a bpf prog,
 		 * which is read only.
 		 */
-		if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1))
-			return -EFAULT;
+		if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1)) {
+			ret = -EFAULT;
+			goto out;
+		}
 		WRITE_ONCE(plt->target, plt_target);
 		set_memory_ro(PAGE_MASK & ((uintptr_t)&plt->target), 1);
 		/* since plt target points to either the new trampoline
@@ -3357,10 +3365,11 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
 	/* if the old target and the new target are both long jumps, no
 	 * patching is required
 	 */
-	if (old_insn == new_insn)
-		return 0;
+	if (old_insn == new_insn) {
+		ret = 0;
+		goto out;
+	}

-	mutex_lock(&text_mutex);
 	if (aarch64_insn_read(ip, &replaced)) {
 		ret = -EFAULT;
 		goto out;
--
2.53.0-Meta

Re: [PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

> bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

TL;DR: Nit only: the new multi-line comment in bpf_arch_text_poke() (arm64
JIT) should put '/*' on its own line, per BPF comment style. No functional
problems found.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c

[ ... ]

> @@ -3338,12 +3338,20 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
>  		 */
>  		plt_target = (u64)&dummy_tramp;
>
> +	/* pages of the bpf prog pack are shared between progs, so the
> +	 * set_memory_rw()/set_memory_ro() window below must be serialized
> +	 * against other pokers too.
> +	 */
> +	mutex_lock(&text_mutex);
> +

This isn't a bug, but should the new multi-line comment put '/*' on a line
by itself, as is the convention for BPF code?

	/*
	 * Pages of the bpf prog pack are shared between progs, so the
	 * set_memory_rw()/set_memory_ro() window below must be serialized
	 * against other pokers too.
	 */

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36966917973

Re: [PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[Puranjay Mohan]

On Fri, Oct 2, 2026 at 5:40 AM Matthew Wood <thepacketgeek@gmail.com> wrote:
>
> Widen the text_mutex critical section in bpf_arch_text_poke() to cover
> the page permission changes made while updating a bpf prog's plt target.
> When a long-jump trampoline is attached to or detached from a bpf prog,
> the plt target at the end of the prog is currently updated before
> text_mutex is taken, by temporarily making the page writable:
>
>         set_memory_rw(page);
>         WRITE_ONCE(plt->target, plt_target);
>         set_memory_ro(page);
>
> Leaving this outside text_mutex assumes the page belongs only to the
> prog being poked, whose pokes are already serialized by its trampoline
> lock. Commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory
> management") broke that assumption; it switched the arm64 JIT to the bpf
> prog pack allocator, which packs programs into small chunks of large
> shared allocations, without updating bpf_arch_text_poke(). Since then,
> unrelated progs (and their plts) commonly share a page. Two CPUs
> attaching to or detaching from different target progs in the same page
> hold different trampoline locks, so their permission changes can
> interleave:
>
>         CPU A                           CPU B
>         set_memory_rw(page)
>                                         set_memory_rw(page)
>                                         WRITE_ONCE(plt_B->target, ...)
>                                         set_memory_ro(page)
>         WRITE_ONCE(plt_A->target, ...)  <- permission fault
>
> This was hit on a 64K page arm64 machine:
>
>   Unable to handle kernel write to read-only memory at virtual address ffff80008f46d5f8
>   FSC = 0x0f: level 3 permission fault
>   pte=00c00200ea5a0783
>   pc : bpf_arch_text_poke+0x214/0x238
>   Call trace:
>    bpf_arch_text_poke+0x214/0x238 (P)
>    __bpf_trampoline_link_prog+0x1c8/0x470
>    bpf_trampoline_link_prog+0x64/0x90
>    bpf_tracing_prog_attach+0x318/0x4a8
>    bpf_raw_tp_link_attach+0x104/0x258
>    bpf_raw_tracepoint_open+0x6c/0x90
>    __sys_bpf+0x134c/0x3e10
>
> Fixes: 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management")
> Signed-off-by: Matthew Wood <thepacketgeek@gmail.com>
> ---
>  arch/arm64/net/bpf_jit_comp.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 475e70653454..66fab0bfd3da 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -3338,12 +3338,20 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type old_t,
>                  */
>                 plt_target = (u64)&dummy_tramp;
>
> +       /* pages of the bpf prog pack are shared between progs, so the
> +        * set_memory_rw()/set_memory_ro() window below must be serialized
> +        * against other pokers too.
> +        */
> +       mutex_lock(&text_mutex);

Thanks for finding this and fixing it.

Reviewed-by: Puranjay Mohan <puranjay@kernel.org>

Alexei,
I am not worried about the comment style, can we merge this?

Thanks,
Puranjay

Re: [PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[Alexei Starovoitov]

On Thu, Oct 01, 2026 at 09:40 PM Matthew Wood <thepacketgeek@gmail.com> wrote:
> +	/* pages of the bpf prog pack are shared between progs, so the
> +	 * set_memory_rw()/set_memory_ro() window below must be serialized
> +	 * against other pokers too.
> +	 */
> +	mutex_lock(&text_mutex);
> +
>  	if (plt_target) {
>  		/* non-zero plt_target indicates we're patching a bpf prog,
>  		 * which is read only.
>  		 */
> -		if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1))
> -			return -EFAULT;
> +		if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
>  		WRITE_ONCE(plt->target, plt_target);
>  		set_memory_ro(PAGE_MASK & ((uintptr_t)&plt->target), 1);

The lock hides the crash, but set_memory_rw() is the actual problem.
The page is shared, so it makes 64K of other progs writable and
executable at the same time.
Use aarch64_insn_write_literal_u64(&plt->target, plt_target) instead.
That's how ftrace_rec_set_ops() and arch_static_call_transform()
update 64-bit literals in the text.
It's atomic and writes via fixmap under patch_lock, just like
aarch64_insn_patch_text_nosync() below.
No need to change page permissions and no need to move text_mutex.

pw-bot: cr

Re: [PATCH] bpf, arm64: Fix text_mutex critical section in bpf_arch_text_poke()

[Matthew Wood]

On Fri, Oct 2, 2026 at 12:21 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Oct 01, 2026 at 09:40 PM Matthew Wood <thepacketgeek@gmail.com> wrote:
> > +     /* pages of the bpf prog pack are shared between progs, so the
> > +      * set_memory_rw()/set_memory_ro() window below must be serialized
> > +      * against other pokers too.
> > +      */
> > +     mutex_lock(&text_mutex);
> > +
> >       if (plt_target) {
> >               /* non-zero plt_target indicates we're patching a bpf prog,
> >                * which is read only.
> >                */
> > -             if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1))
> > -                     return -EFAULT;
> > +             if (set_memory_rw(PAGE_MASK & ((uintptr_t)&plt->target), 1)) {
> > +                     ret = -EFAULT;
> > +                     goto out;
> > +             }
> >               WRITE_ONCE(plt->target, plt_target);
> >               set_memory_ro(PAGE_MASK & ((uintptr_t)&plt->target), 1);
>
> The lock hides the crash, but set_memory_rw() is the actual problem.
> The page is shared, so it makes 64K of other progs writable and
> executable at the same time.
> Use aarch64_insn_write_literal_u64(&plt->target, plt_target) instead.
> That's how ftrace_rec_set_ops() and arch_static_call_transform()
> update 64-bit literals in the text.
> It's atomic and writes via fixmap under patch_lock, just like
> aarch64_insn_patch_text_nosync() below.
> No need to change page permissions and no need to move text_mutex.
>
> pw-bot: cr

Thank you both for the quick review! This
aarch64_insn_write_literal_u64 makes sense,
I'll test and submit an update shortly.

Regards,
Matthew