[PATCH bpf 0/2] bpf: Shorten pack_mutex critical sections

[PATCH bpf 0/2] bpf: Shorten pack_mutex critical sections

[Puranjay Mohan]

Patch 2 moves bpf_arch_text_invalidate() out of pack_mutex, as
suggested on v1 [1].

That makes the frees fast enough to hit a deadlock that was already
there: the pack_mutex owner frees an empty pack and waits in
flush_work() for vmalloc purge work, which cannot run while
bpf_prog_free_deferred() items waiting for pack_mutex fill the
system_percpu_wq pool. Patch 1 allocates and frees pack memory without
pack_mutex. Seen on arm64; x86 and powerpc take the same path.

Changes since v1:
- Invalidate before taking pack_mutex on all archs, instead of dropping
  and retaking it on arm64
- New patch 1 for the deadlock

[1] https://lore.kernel.org/bpf/20261024031046.625578-1-alexei.starovoitov@gmail.com/

Puranjay Mohan (2):
  bpf: Don't allocate and free prog pack memory with pack_mutex held
  bpf: Invalidate freed prog pack memory without holding pack_mutex

 kernel/bpf/core.c | 58 +++++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 19 deletions(-)


base-commit: 9abe2c42ecab6c86b54838d2d645ca2e10204606
-- 
2.53.0-Meta

[PATCH bpf 1/2] bpf: Don't allocate and free prog pack memory with pack_mutex held

[Puranjay Mohan]

bpf_prog_pack_free() frees an empty pack with pack_mutex held, and
alloc_new_pack() calls set_memory_rox() with it held. Both can end up in
__purge_vmap_area_lazy(), which queues purge_work on system_percpu_wq
and waits for it since commit 72210662c5a2 ("mm: vmalloc: offload
free_vmap_area_lock lock"). set_memory_rox() gets there through
vm_unmap_aliases() on arm64, x86 and powerpc.

Progs are freed from system_percpu_wq too, and bpf_prog_free_deferred()
sleeps on pack_mutex. When enough progs are freed at once, these work
items take all active slots of the pool that purge_work was queued to.
purge_work never runs and everything waits for pack_mutex:

  INFO: task kworker/0:0:9 blocked for more than 20 seconds.
  Workqueue: events bpf_prog_free_deferred
  Call trace:
   ...
   mutex_lock+0x50/0x68
   bpf_prog_pack_free+0x50/0x270
   bpf_jit_binary_pack_free+0x28/0xb0
   bpf_jit_free+0x58/0x1c8
   bpf_prog_free_images+0x84/0xa0
   bpf_prog_free_deferred+0x198/0x1d8
   process_one_work+0x1a0/0x490
   ...
  INFO: task kworker/0:0:9 is blocked on a mutex likely owned by task kworker/9:25:646.

while kworker/9:25 is in:

  __flush_work+0x78/0xf8
  flush_work+0x1c/0x30
  __purge_vmap_area_lazy+0x420/0x470
  _vm_unmap_aliases+0x21c/0x2b0
  vfree+0x1d0/0x250
  execmem_free+0x2c/0x58
  bpf_prog_pack_free+0x1a8/0x270
  bpf_jit_binary_pack_free+0x28/0xb0
  bpf_jit_free+0x58/0x1c8
  bpf_prog_free_images+0x84/0xa0
  bpf_prog_free_deferred+0x198/0x1d8
  process_one_work+0x1a0/0x490

This was hit in 16 cpu arm64 KVM guests by processes that attach cBPF
filters to sockets and close them in a loop, with
bpf_arch_text_invalidate() moved out of pack_mutex so that the frees are
faster.

pack_mutex protects pack_list, the bitmaps and arch_flush_needed. Set
up a new pack without it, then look for room again, since another
allocator may have added a pack in the meantime, and free the new pack
if it is not needed. Free the memory of an empty pack after taking it
off the list and dropping pack_mutex. Allocations larger than a pack
touch none of this, so don't take pack_mutex for them.

Fixes: 72210662c5a2 ("mm: vmalloc: offload free_vmap_area_lock lock")
Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 kernel/bpf/core.c | 52 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 831f73f825993..4dac33a4208aa 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -951,7 +951,6 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 		if (err)
 			goto out;
 	}
-	list_add_tail(&pack->list, &pack_list);
 	return pack;
 
 out:
@@ -963,11 +962,10 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns, bool was_classic)
 {
 	unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
-	struct bpf_prog_pack *pack, *fallback_pack = NULL;
-	unsigned long pos, fallback_pos = 0;
+	struct bpf_prog_pack *pack, *fallback_pack, *new_pack = NULL;
+	unsigned long pos, fallback_pos;
 	void *ptr = NULL;
 
-	mutex_lock(&pack_mutex);
 	if (size > BPF_PROG_PACK_SIZE) {
 		/*
 		 * Allocations larger than a pack get their own pages, and
@@ -991,8 +989,13 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns, bool
 				ptr = NULL;
 			}
 		}
-		goto out;
+		return ptr;
 	}
+
+	mutex_lock(&pack_mutex);
+again:
+	fallback_pack = NULL;
+	fallback_pos = 0;
 	list_for_each_entry(pack, &pack_list, list) {
 		pos = bitmap_find_next_zero_area(pack->bitmap, BPF_PROG_CHUNK_COUNT, 0,
 						 nbits, 0);
@@ -1021,11 +1024,21 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns, bool
 		goto found_free_area;
 	}
 
-	pack = alloc_new_pack(bpf_fill_ill_insns);
-	if (!pack)
-		goto out;
+	if (new_pack) {
+		list_add_tail(&new_pack->list, &pack_list);
+		pack = new_pack;
+		new_pack = NULL;
+		pos = 0;
+		goto found_free_area;
+	}
 
-	pos = 0;
+	mutex_unlock(&pack_mutex);
+	new_pack = alloc_new_pack(bpf_fill_ill_insns);
+	if (!new_pack)
+		return NULL;
+
+	mutex_lock(&pack_mutex);
+	goto again;
 
 found_free_area:
 	/* Flush only for cBPF as it may contain a crafted gadget */
@@ -1040,9 +1053,12 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns, bool
 	}
 	bitmap_set(pack->bitmap, pos, nbits);
 	ptr = (void *)(pack->ptr) + (pos << BPF_PROG_CHUNK_SHIFT);
-
-out:
 	mutex_unlock(&pack_mutex);
+
+	if (new_pack) {
+		bpf_jit_free_exec(new_pack->ptr);
+		kfree(new_pack);
+	}
 	return ptr;
 }
 
@@ -1052,12 +1068,12 @@ void bpf_prog_pack_free(void *ptr, u32 size)
 	unsigned int nbits;
 	unsigned long pos;
 
-	mutex_lock(&pack_mutex);
 	if (size > BPF_PROG_PACK_SIZE) {
 		bpf_jit_free_exec(ptr);
-		goto out;
+		return;
 	}
 
+	mutex_lock(&pack_mutex);
 	list_for_each_entry(tmp, &pack_list, list) {
 		if (ptr >= tmp->ptr && (tmp->ptr + BPF_PROG_PACK_SIZE) > ptr) {
 			pack = tmp;
@@ -1079,13 +1095,17 @@ void bpf_prog_pack_free(void *ptr, u32 size)
 	if (static_branch_unlikely(&bpf_pred_flush_enabled))
 		pack->arch_flush_needed = true;
 	if (bitmap_find_next_zero_area(pack->bitmap, BPF_PROG_CHUNK_COUNT, 0,
-				       BPF_PROG_CHUNK_COUNT, 0) == 0) {
+				       BPF_PROG_CHUNK_COUNT, 0) == 0)
 		list_del(&pack->list);
+	else
+		pack = NULL;
+out:
+	mutex_unlock(&pack_mutex);
+
+	if (pack) {
 		bpf_jit_free_exec(pack->ptr);
 		kfree(pack);
 	}
-out:
-	mutex_unlock(&pack_mutex);
 }
 
 static atomic_long_t bpf_jit_current;
-- 
2.53.0-Meta

Re: [PATCH bpf 1/2] bpf: Don't allocate and free prog pack memory with pack_mutex held

[bot+bpf-ci]

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

> bpf: Don't allocate and free prog pack memory with pack_mutex held

TL;DR:
- bpf_prog_pack_alloc() may return NULL without rescanning pack_list if its
  unlocked alloc fails while another allocator added a pack, so JIT or
  trampoline allocation could spuriously fail
- Commit message likely wrongly lists powerpc

> bpf: Don't allocate and free prog pack memory with pack_mutex held
>
>     bpf_prog_pack_free() frees an empty pack with pack_mutex held, and
>     alloc_new_pack() calls set_memory_rox() with it held. Both can end up in
>     __purge_vmap_area_lazy(), which queues purge_work on system_percpu_wq
>     and waits for it since commit 72210662c5a2 ("mm: vmalloc: offload
>     free_vmap_area_lock lock"). set_memory_rox() gets there through
>     vm_unmap_aliases() on arm64, x86 and powerpc.

Is the powerpc part of this correct?  On x86 and arm64 set_memory_rox()
reaches vm_unmap_aliases() via change_page_attr_set_clr() and
change_memory_common() respectively.

On powerpc, set_memory_rox() is change_memory_attr(), which only calls
apply_to_existing_page_range() with change_page_attr() and then
flush_tlb_kernel_range().  The only vm_unmap_aliases() call under
arch/powerpc is in the memory hot-remove path.

On powerpc only the free path (vfree() -> vm_reset_perms() ->
_vm_unmap_aliases()) seems to reach __purge_vmap_area_lazy().  Should
powerpc be dropped from that sentence, or should it say which path applies?

[ ... ]

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c

[ ... ]

> @@ -1021,11 +1024,21 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns, bool
>  		goto found_free_area;
>  	}
>  
> -	pack = alloc_new_pack(bpf_fill_ill_insns);
> -	if (!pack)
> -		goto out;
> +	if (new_pack) {
> +		list_add_tail(&new_pack->list, &pack_list);
> +		pack = new_pack;
> +		new_pack = NULL;
> +		pos = 0;
> +		goto found_free_area;
> +	}
>  
> -	pos = 0;
> +	mutex_unlock(&pack_mutex);
> +	new_pack = alloc_new_pack(bpf_fill_ill_insns);
> +	if (!new_pack)
> +		return NULL;
> +
> +	mutex_lock(&pack_mutex);
> +	goto again;

Can this return NULL when another allocator has just added a pack with
free space?

The decision to allocate is made on a pack_list snapshot that is stale once
pack_mutex is dropped.  The success path re-checks pack_list after
re-locking, but the failure path returns NULL right away.

Suppose allocators A and B both find no room and both call
alloc_new_pack() without the mutex.  A succeeds and B fails, for example
because execmem_vmalloc() cannot find space.  On x86 the EXECMEM_BPF range
is the module area, about 1GB shared with modules, with no fallback range.
B returns NULL even though A's pack, added to pack_list moments later, has
BPF_PROG_CHUNK_COUNT free chunks.

Before this patch alloc_new_pack() ran under pack_mutex, so B would have
waited for the mutex, found room in A's pack and succeeded without
allocating.

This also seems to make such failures more likely.  Every concurrent
allocator that sees no room now allocates its own BPF_PROG_PACK_SIZE
(SZ_2M * num_possible_nodes()) region at the same time, before the extra
ones are freed.  On a large NUMA x86 host that transient demand could be
many tens of MB to hundreds of MB of module space, for example when many
seccomp or socket filters are JITed at once after the last pack was freed.

The NULL reaches bpf_jit_binary_pack_alloc() and then the arch JITs (the
x86 do_jit path does 'if (!header) goto out_addrs;'), so the program falls
back to the interpreter or fails to load with CONFIG_BPF_JIT_ALWAYS_ON.
Trampoline and struct_ops image allocation fail with -ENOMEM
(bpf_tramp_image_alloc(), bpf_struct_ops_image_alloc()).

Would it be better to re-take pack_mutex and scan pack_list once more when
alloc_new_pack() fails, before returning NULL?  The later commit
653557885326 ("bpf: Invalidate freed prog pack memory without holding
pack_mutex") in the series does not touch this path.


---
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/41657960013

Re: [PATCH bpf 1/2] bpf: Don't allocate and free prog pack memory with pack_mutex held

[Alexei Starovoitov]

On Tue, Nov 03, 2026 at 06:28 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> Progs are freed from system_percpu_wq too, and bpf_prog_free_deferred()
> sleeps on pack_mutex. When enough progs are freed at once, these work
> items take all active slots of the pool that purge_work was queued to.
> purge_work never runs and everything waits for pack_mutex:

Thanks for taking over.

Turned out Emil is fixing the same issue I was seeing:
https://lore.kernel.org/bpf/20261102073439.6822-1-emil@etsalapatis.com/

Thousands of work items that sleep don't belong in system_percpu_wq.
commit 117932eea99b fixed it for cgroup/bpf.

On Emil's patch I proposed to move frees into their own wq
with max_active = 1.

                                         this patch    own wq
  -------------------------------------  ------------  ----------
  work items per cpu that sleep on       up to 1024    1
  pack_mutex
  same deadlock on vmap_purge_lock       still there   gone
  N tasks find no room in the packs      N new packs   1 new pack
  pack_mutex held over execmem calls     no            yes

Row 1 also bounds the time that a prog load waits for pack_mutex.

Row 2: the s390 JIT and the interpreter call vfree() of
a VM_FLUSH_RESET_PERMS area for every prog.
vm_reset_perms() calls _vm_unmap_aliases(), so these work items sleep
on vmap_purge_lock. Its owner waits for purge_work in the same wq.

Row 3: see below.

Row 4 is fine. purge_work gets its slot, so the owner of pack_mutex
makes progress.

Then no need for the retry in bpf_prog_pack_alloc().

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fdbf638..6ce2e31 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -70,6 +70,7 @@ extern spinlock_t btf_idr_lock;
 extern struct kobject *btf_kobj;
 extern struct bpf_mem_alloc bpf_global_ma, bpf_global_percpu_ma;
 extern bool bpf_global_ma_set;
+extern struct workqueue_struct *bpf_free_wq;

 typedef u64 (*bpf_callback_t)(u64, u64, u64, u64, u64);
 typedef int (*bpf_iter_init_seq_priv_t)(void *private_data,
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 831f73f..e83edea 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3198,7 +3198,7 @@ static void bpf_prog_free_deferred(struct work_struct *work)
 	/* The symbols are looked up under rcu_read_lock() */
 	INIT_RCU_WORK(&aux->rwork, bpf_prog_free_images);
 	if (ksyms)
-		queue_rcu_work(system_percpu_wq, &aux->rwork);
+		queue_rcu_work(bpf_free_wq, &aux->rwork);
 	else
 		bpf_prog_free_images(&aux->rwork.work);
 }
@@ -3211,9 +3211,26 @@ void bpf_prog_free(struct bpf_prog *fp)
 		bpf_prog_put(aux->dst_prog);
 	bpf_token_put(aux->token);
 	INIT_WORK(&aux->work, bpf_prog_free_deferred);
-	schedule_work(&aux->work);
+	queue_work(bpf_free_wq, &aux->work);
 }

+/*
+ * Many progs and trampolines can be freed at once and their work items sleep
+ * on pack_mutex. Its owner can wait for a work item of system_percpu_wq.
+ * Use a separate workqueue, so that they don't fill up max_active of
+ * system_percpu_wq.
+ */
+struct workqueue_struct *bpf_free_wq;
+
+static int __init bpf_free_wq_init(void)
+{
+	bpf_free_wq = alloc_workqueue("bpf_free", WQ_PERCPU, 1);
+	if (!bpf_free_wq)
+		panic("Failed to alloc workqueue for bpf free.\n");
+	return 0;
+}
+pure_initcall(bpf_free_wq_init);
+
 /* RNG for unprivileged user space with separated state from prandom_u32(). */
 static DEFINE_PER_CPU(struct rnd_state, bpf_user_rnd_state);

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 21d4608..a34e8a2 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -584,7 +584,7 @@ static void __bpf_tramp_image_put_rcu(struct rcu_head *rcu)

 	im = container_of(rcu, struct bpf_tramp_image, rcu);
 	INIT_WORK(&im->work, __bpf_tramp_image_put_deferred);
-	schedule_work(&im->work);
+	queue_work(bpf_free_wq, &im->work);
 }

 /* callback, fexit step 2. Called after percpu_ref_kill confirms. */

> +	mutex_unlock(&pack_mutex);
> +	new_pack = alloc_new_pack(bpf_fill_ill_insns);
> +	if (!new_pack)
> +		return NULL;
> +
> +	mutex_lock(&pack_mutex);
> +	goto again;

Every task that finds no room before task 1 adds its pack allocates
one more:

  task 1                     task 2 ... task N
  no room, unlock
  alloc_new_pack()           no room, unlock
                             alloc_new_pack()
  lock, list_add_tail()
                             lock, room in the pack of task 1
                             unlock, free the new pack

A pack is 2 Mbytes * num_possible_nodes(). Nobody is charged for it.
N is the number of tasks, not cpus, since alloc_new_pack() sleeps.
If we go with your approach the memory needs to be bound somehow.

pw-bot: cr

Re: [PATCH bpf 1/2] bpf: Don't allocate and free prog pack memory with pack_mutex held

[Emil Tsalapatis]

On Tue, Nov 3, 2026 at 4:54 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 03, 2026 at 06:28 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> > Progs are freed from system_percpu_wq too, and bpf_prog_free_deferred()
> > sleeps on pack_mutex. When enough progs are freed at once, these work
> > items take all active slots of the pool that purge_work was queued to.
> > purge_work never runs and everything waits for pack_mutex:
>
> Thanks for taking over.
>
> Turned out Emil is fixing the same issue I was seeing:
> https://lore.kernel.org/bpf/20261102073439.6822-1-emil@etsalapatis.com/
>
> Thousands of work items that sleep don't belong in system_percpu_wq.
> commit 117932eea99b fixed it for cgroup/bpf.
>

Sorry for the delay, I had v2 ready and hadn't sent it. I chatted with Puranjay
and am sending it now. If we prefer the approach below he can take over
instead.

> On Emil's patch I proposed to move frees into their own wq
> with max_active = 1.
>
>                                          this patch    own wq
>   -------------------------------------  ------------  ----------
>   work items per cpu that sleep on       up to 1024    1
>   pack_mutex
>   same deadlock on vmap_purge_lock       still there   gone
>   N tasks find no room in the packs      N new packs   1 new pack
>   pack_mutex held over execmem calls     no            yes
>
> Row 1 also bounds the time that a prog load waits for pack_mutex.
>
> Row 2: the s390 JIT and the interpreter call vfree() of
> a VM_FLUSH_RESET_PERMS area for every prog.
> vm_reset_perms() calls _vm_unmap_aliases(), so these work items sleep
> on vmap_purge_lock. Its owner waits for purge_work in the same wq.
>
> Row 3: see below.
>
> Row 4 is fine. purge_work gets its slot, so the owner of pack_mutex
> makes progress.
>
> Then no need for the retry in bpf_prog_pack_alloc().
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index fdbf638..6ce2e31 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -70,6 +70,7 @@ extern spinlock_t btf_idr_lock;
>  extern struct kobject *btf_kobj;
>  extern struct bpf_mem_alloc bpf_global_ma, bpf_global_percpu_ma;
>  extern bool bpf_global_ma_set;
> +extern struct workqueue_struct *bpf_free_wq;
>
>  typedef u64 (*bpf_callback_t)(u64, u64, u64, u64, u64);
>  typedef int (*bpf_iter_init_seq_priv_t)(void *private_data,
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 831f73f..e83edea 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -3198,7 +3198,7 @@ static void bpf_prog_free_deferred(struct work_struct *work)
>         /* The symbols are looked up under rcu_read_lock() */
>         INIT_RCU_WORK(&aux->rwork, bpf_prog_free_images);
>         if (ksyms)
> -               queue_rcu_work(system_percpu_wq, &aux->rwork);
> +               queue_rcu_work(bpf_free_wq, &aux->rwork);
>         else
>                 bpf_prog_free_images(&aux->rwork.work);
>  }
> @@ -3211,9 +3211,26 @@ void bpf_prog_free(struct bpf_prog *fp)
>                 bpf_prog_put(aux->dst_prog);
>         bpf_token_put(aux->token);
>         INIT_WORK(&aux->work, bpf_prog_free_deferred);
> -       schedule_work(&aux->work);
> +       queue_work(bpf_free_wq, &aux->work);
>  }
>
> +/*
> + * Many progs and trampolines can be freed at once and their work items sleep
> + * on pack_mutex. Its owner can wait for a work item of system_percpu_wq.
> + * Use a separate workqueue, so that they don't fill up max_active of
> + * system_percpu_wq.
> + */
> +struct workqueue_struct *bpf_free_wq;
> +
> +static int __init bpf_free_wq_init(void)
> +{
> +       bpf_free_wq = alloc_workqueue("bpf_free", WQ_PERCPU, 1);
> +       if (!bpf_free_wq)
> +               panic("Failed to alloc workqueue for bpf free.\n");
> +       return 0;
> +}
> +pure_initcall(bpf_free_wq_init);
> +
>  /* RNG for unprivileged user space with separated state from prandom_u32(). */
>  static DEFINE_PER_CPU(struct rnd_state, bpf_user_rnd_state);
>
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 21d4608..a34e8a2 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -584,7 +584,7 @@ static void __bpf_tramp_image_put_rcu(struct rcu_head *rcu)
>
>         im = container_of(rcu, struct bpf_tramp_image, rcu);
>         INIT_WORK(&im->work, __bpf_tramp_image_put_deferred);
> -       schedule_work(&im->work);
> +       queue_work(bpf_free_wq, &im->work);
>  }
>
>  /* callback, fexit step 2. Called after percpu_ref_kill confirms. */
>
> > +     mutex_unlock(&pack_mutex);
> > +     new_pack = alloc_new_pack(bpf_fill_ill_insns);
> > +     if (!new_pack)
> > +             return NULL;
> > +
> > +     mutex_lock(&pack_mutex);
> > +     goto again;
>
> Every task that finds no room before task 1 adds its pack allocates
> one more:
>
>   task 1                     task 2 ... task N
>   no room, unlock
>   alloc_new_pack()           no room, unlock
>                              alloc_new_pack()
>   lock, list_add_tail()
>                              lock, room in the pack of task 1
>                              unlock, free the new pack
>
> A pack is 2 Mbytes * num_possible_nodes(). Nobody is charged for it.
> N is the number of tasks, not cpus, since alloc_new_pack() sleeps.
> If we go with your approach the memory needs to be bound somehow.
>
> pw-bot: cr
>

[PATCH bpf 2/2] bpf: Invalidate freed prog pack memory without holding pack_mutex

[Puranjay Mohan]

bpf_prog_pack_free() calls bpf_arch_text_invalidate() with pack_mutex
held. On arm64 it interrupts all other CPUs and waits for them:

  bpf_prog_free_deferred
    bpf_prog_free_images
      bpf_jit_free
        bpf_jit_binary_pack_free
          bpf_prog_pack_free
            bpf_arch_text_invalidate
              aarch64_insn_set
                __text_poke
                  flush_icache_range
                    kick_all_cpus_sync

When many progs are freed at once, every prog load waits in
bpf_prog_pack_alloc() for all of these broadcasts. On arm64 hosts with
many CPUs this showed up as hung task reports with sk_attach_filter()
blocked on pack_mutex for minutes.

pack_mutex protects pack_list, the bitmaps and arch_flush_needed, not
the text. Until bitmap_clear(), the bits of the range are set, so it
cannot be allocated again and the pack cannot be freed. Invalidate it
before taking pack_mutex. A ptr that is in no pack, which only a caller
bug can pass, is now written to before the lookup warns about it.

Tested in arm64 KVM guests with 16 and 64 cpus on a 72 cpu host. cpus/2
processes attach a cBPF filter to 256 sockets and close them in a loop
for 20 sec. One more process attaches a filter every msec. Median of 5
runs.

Sockets attached and closed in 20 sec, in millions (higher is better):

             before  after
  16 cpus      1.22   3.79
  64 cpus      0.42   3.38

setsockopt(SO_ATTACH_FILTER) of the process that attaches every msec,
in msec (lower is better):

                16 cpus          64 cpus
             before  after    before  after
  avg          1.32   0.27      7.21   0.56
  p99          5.39   5.52      79.7   9.89
  max           447    159      1598    253

Fixes: 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
 kernel/bpf/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 4dac33a4208aa..51f1b128f0d63 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1073,6 +1073,9 @@ void bpf_prog_pack_free(void *ptr, u32 size)
 		return;
 	}
 
+	WARN_ONCE(bpf_arch_text_invalidate(ptr, size),
+		  "bpf_prog_pack bug: missing bpf_arch_text_invalidate?\n");
+
 	mutex_lock(&pack_mutex);
 	list_for_each_entry(tmp, &pack_list, list) {
 		if (ptr >= tmp->ptr && (tmp->ptr + BPF_PROG_PACK_SIZE) > ptr) {
@@ -1087,9 +1090,6 @@ void bpf_prog_pack_free(void *ptr, u32 size)
 	nbits = BPF_PROG_SIZE_TO_NBITS(size);
 	pos = ((unsigned long)ptr - (unsigned long)pack->ptr) >> BPF_PROG_CHUNK_SHIFT;
 
-	WARN_ONCE(bpf_arch_text_invalidate(ptr, size),
-		  "bpf_prog_pack bug: missing bpf_arch_text_invalidate?\n");
-
 	bitmap_clear(pack->bitmap, pos, nbits);
 
 	if (static_branch_unlikely(&bpf_pred_flush_enabled))
-- 
2.53.0-Meta