Merge branch 'bpf-fix-objects-stuck-in-free_by_rcu_ttrace'

Alexei Starovoitov says:

====================
bpf: Fix objects stuck in free_by_rcu_ttrace

From: Alexei Starovoitov <ast@kernel.org>

The objects that bpf_mem_alloc frees while RCU tasks trace GP is in flight
stay in free_by_rcu_ttrace list until the same bpf_mem_cache frees or
allocates in bulk again.

Patch 1 - refactoring. No functional change.
Patch 2 - the fix.
Patch 3 - selftest.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
====================

Link: https://patch.msgid.link/20260930095920.601738-1-alexei.starovoitov@gmail.com
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

kernel/bpf/memalloc.c

@@ -118,6 +118,11 @@ struct bpf_mem_cache {
 	struct llist_head free_by_rcu_ttrace;
 	struct llist_head waiting_for_gp_ttrace;
 	struct rcu_head rcu_ttrace;
+	/*
+	 * 0 - idle
+	 * 1 - __free_rcu() is queued
+	 * 2 - __free_rcu() is queued and free_by_rcu_ttrace got more objects since
+	 */
 	atomic_t call_rcu_ttrace_in_progress;
 	raw_spinlock_t lock;
 };
@@ -276,6 +281,8 @@ static int free_all(struct bpf_mem_cache *c, struct llist_node *llnode, bool per
 	return cnt;
 }
 
+static void __do_call_rcu_ttrace(struct bpf_mem_cache *c);
+
 static void __free_rcu(struct rcu_head *head)
 {
 	struct bpf_mem_cache *c = container_of(head, struct bpf_mem_cache, rcu_ttrace);
@@ -285,7 +292,19 @@ static void __free_rcu(struct rcu_head *head)
 		llnode = llist_del_all(&c->waiting_for_gp_ttrace);
 
 	free_all(c, llnode, !!c->percpu_size);
-	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
+
+	/*
+	 * do_call_rcu_ttrace() that ran while GP was in flight left its objects
+	 * in free_by_rcu_ttrace. This cache may never free or alloc in bulk
+	 * again, so start the next GP from here.
+	 * 'c' can be freed as soon as call_rcu_ttrace_in_progress is zero.
+	 */
+	if (atomic_cmpxchg(&c->call_rcu_ttrace_in_progress, 1, 0) == 1)
+		return;
+
+	/* Pairs with synchronize_rcu() in free_mem_alloc() */
+	guard(rcu)();
+	__do_call_rcu_ttrace(c);
 }
 
 static void enque_to_free(struct bpf_mem_cache *c, void *obj)
@@ -298,18 +317,15 @@ static void enque_to_free(struct bpf_mem_cache *c, void *obj)
 	llist_add(llnode, &c->free_by_rcu_ttrace);
 }
 
-static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
+static void __do_call_rcu_ttrace(struct bpf_mem_cache *c)
 {
 	struct llist_node *llnode, *t;
 
-	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
-		if (unlikely(READ_ONCE(c->draining))) {
-			scoped_guard(raw_spinlock_irqsave, &c->lock)
-				llnode = llist_del_all(&c->free_by_rcu_ttrace);
-			free_all(c, llnode, !!c->percpu_size);
-		}
-		return;
-	}
+	/*
+	 * Must be done before llist_del_all(). Objects that it misses were
+	 * added by do_call_rcu_ttrace() that will set 2 after this store.
+	 */
+	atomic_set(&c->call_rcu_ttrace_in_progress, 1);
 
 	WARN_ON_ONCE(!llist_empty(&c->waiting_for_gp_ttrace));
 	llist_for_each_safe(llnode, t, llist_del_all(&c->free_by_rcu_ttrace))
@@ -328,6 +344,22 @@ static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
 	call_rcu_tasks_trace(&c->rcu_ttrace, __free_rcu);
 }
 
+static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
+{
+	struct llist_node *llnode;
+
+	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 2)) {
+		if (unlikely(READ_ONCE(c->draining))) {
+			scoped_guard(raw_spinlock_irqsave, &c->lock)
+				llnode = llist_del_all(&c->free_by_rcu_ttrace);
+			free_all(c, llnode, !!c->percpu_size);
+		}
+		return;
+	}
+
+	__do_call_rcu_ttrace(c);
+}
+
 static void free_bulk(struct bpf_mem_cache *c)
 {
 	struct bpf_mem_cache *tgt = c->tgt;
@@ -700,7 +732,12 @@ static void free_mem_alloc(struct bpf_mem_alloc *ma)
 	 * to wait for the pending __free_by_rcu(), and __free_rcu(). RCU Tasks
 	 * Trace grace period implies RCU grace period, so all __free_rcu don't
 	 * need extra call_rcu() (and thus extra rcu_barrier() here).
+	 *
+	 * __free_rcu() queues itself again unless it sees 'draining'. After
+	 * synchronize_rcu() it either did that already or will not do it, so
+	 * rcu_barrier_tasks_trace() cannot miss it.
 	 */
+	synchronize_rcu();
 	rcu_barrier(); /* wait for __free_by_rcu */
 	rcu_barrier_tasks_trace(); /* wait for __free_rcu */
 	free_mem_alloc_no_barrier(ma);

tools/testing/selftests/bpf/prog_tests/bpf_ma_ttrace.c

@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "bpf_ma_ttrace.skel.h"
+
+#define NR_ELEMS 4096
+
+/*
+ * The first free_bulk() starts RCU tasks trace GP. The rest of the elements are
+ * deleted while it's in flight. They should be freed without further alloc or
+ * free from this map.
+ */
+void test_bpf_ma_ttrace(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+	struct bpf_ma_ttrace *skel;
+	__u32 cnt = NR_ELEMS;
+	long *vals = NULL;
+	int *keys = NULL;
+	int i, err, fd, nr_cpus;
+
+	skel = bpf_ma_ttrace__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+	nr_cpus = libbpf_num_possible_cpus();
+	if (!ASSERT_GT(nr_cpus, 0, "nr_cpus"))
+		goto out;
+	skel->bss->nr_cpus = nr_cpus;
+
+	keys = calloc(NR_ELEMS, sizeof(*keys));
+	vals = calloc(NR_ELEMS, sizeof(*vals));
+	if (!ASSERT_OK_PTR(keys, "keys") || !ASSERT_OK_PTR(vals, "vals"))
+		goto out;
+	for (i = 0; i < NR_ELEMS; i++)
+		keys[i] = i;
+
+	fd = bpf_map__fd(skel->maps.htab);
+	err = bpf_map_update_batch(fd, keys, vals, &cnt, NULL);
+	if (!ASSERT_OK(err, "update_batch") || !ASSERT_EQ(cnt, NR_ELEMS, "update_cnt"))
+		goto out;
+	err = bpf_map_delete_batch(fd, keys, &cnt, NULL);
+	if (!ASSERT_OK(err, "delete_batch") || !ASSERT_EQ(cnt, NR_ELEMS, "delete_cnt"))
+		goto out;
+
+	/* Wait for all __free_rcu() callbacks to finish */
+	for (i = 0; i < 300; i++) {
+		err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.check_ttrace), &opts);
+		if (!ASSERT_OK(err, "test_run") || !ASSERT_OK(opts.retval, "retval"))
+			goto out;
+		if (!skel->bss->in_progress)
+			break;
+		usleep(100000);
+	}
+	ASSERT_EQ(skel->bss->nr_caches, nr_cpus, "nr_caches");
+	ASSERT_EQ(skel->bss->in_progress, 0, "in_progress");
+	ASSERT_EQ(skel->bss->not_freed, 0, "not_freed");
+out:
+	free(keys);
+	free(vals);
+	bpf_ma_ttrace__destroy(skel);
+}

tools/testing/selftests/bpf/progs/bpf_ma_ttrace.c

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+#include "bpf_kfuncs.h"
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__uint(max_entries, 4096);
+	__type(key, int);
+	__type(value, long);
+} htab SEC(".maps");
+
+extern const void __per_cpu_offset __ksym;
+
+int nr_cpus;
+int nr_caches;
+int in_progress;
+int not_freed;
+
+/* Look at bpf_mem_cache of every cpu that htab allocates its elements from */
+SEC("syscall")
+int check_ttrace(void *ctx)
+{
+	struct bpf_htab *h = bpf_core_cast(&htab, struct bpf_htab);
+	unsigned long cache = (unsigned long)BPF_CORE_READ(h, ma.cache);
+	const unsigned long *offsets = &__per_cpu_offset;
+	struct bpf_mem_cache *c;
+	unsigned long off;
+	int cpu;
+
+	nr_caches = 0;
+	in_progress = 0;
+	not_freed = 0;
+	bpf_for(cpu, 0, nr_cpus) {
+		if (bpf_probe_read_kernel(&off, sizeof(off), offsets + cpu))
+			return -1;
+		c = bpf_core_cast((void *)(cache + off), struct bpf_mem_cache);
+		if (c->unit_size)
+			nr_caches++;
+		if (c->call_rcu_ttrace_in_progress.counter)
+			in_progress++;
+		if (c->free_by_rcu_ttrace.first || c->waiting_for_gp_ttrace.first)
+			not_freed++;
+	}
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";