bpf: Register dtor for freeing special fields

There is a race window where BPF hash map elements can leak special fields if the program with access to the map value recreates these special fields between the check_and_free_fields done on the map value and its eventual return to the memory allocator. Several ways were explored prior to this patch, most notably [0] tried to use a poison value to reject attempts to recreate special fields for map values that have been logically deleted but still accessible to BPF programs (either while sitting in the free list or when reused). While this approach works well for task work, timers, wq, etc., it is harder to apply the idea to kptrs, which have a similar race and failure mode. Instead, we change bpf_mem_alloc to allow registering destructor for allocated elements, such that when they are returned to the allocator, any special fields created while they were accessible to programs in the mean time will be freed. If these values get reused, we do not free the fields again before handing the element back. The special fields thus may remain initialized while the map value sits in a free list. When bpf_mem_alloc is retired in the future, a similar concept can be introduced to kmalloc_nolock-backed kmem_cache, paired with the existing idea of a constructor. Note that the destructor registration happens in map_check_btf, after the BTF record is populated and (at that point) avaiable for inspection and duplication. Duplication is necessary since the freeing of embedded bpf_mem_alloc can be decoupled from actual map lifetime due to logic introduced to reduce the cost of rcu_barrier()s in mem alloc free path in 9f2c6e96c65e ("bpf: Optimize rcu_barrier usage between hash map and bpf_mem_alloc."). As such, once all callbacks are done, we must also free the duplicated record. To remove dependency on the bpf_map itself, also stash the key size of the map to obtain value from htab_elem long after the map is gone. [0]: https://lore.kernel.org/bpf/20260216131341.1285427-1-mykyta.yatsenko5@gmail.com

Kumar Kartikeya Dwivedi committed 8 months ago

Commit 1df97a71 parent 6881af23 files · +140 −11Browse files

Showing the changes to kernel (2 of 3 files). Show all

Unified · Split

2 files

kernel/bpf/hashtab.c +87 −0
kernel/bpf/memalloc.c +47 −11

kernel/bpf/hashtab.c+87−0

@@ -125,6 +125,11 @@ struct htab_elem { · 124 unmodified lines
125	char key[] __aligned(8);	125	char key[] __aligned(8);
126	};	126	};
127		127
		128	struct htab_btf_record {
		129	struct btf_record *record;
		130	u32 key_size;
		131	};
		132
128	static inline bool htab_is_prealloc(const struct bpf_htab *htab)	133	static inline bool htab_is_prealloc(const struct bpf_htab *htab)
129	{	134	{
130	return !(htab->map.map_flags & BPF_F_NO_PREALLOC);	135	return !(htab->map.map_flags & BPF_F_NO_PREALLOC);
@@ -457,6 +462,84 @@ static int htab_map_alloc_check(union bpf_attr *attr) · 326 unmodified lines
457	return 0;	462	return 0;
458	}	463	}
459		464
		465	static void htab_mem_dtor(void obj, void ctx)
		466	{
		467	struct htab_btf_record *hrec = ctx;
		468	struct htab_elem *elem = obj;
		469	void *map_value;
		470
		471	if (IS_ERR_OR_NULL(hrec->record))
		472	return;
		473
		474	map_value = htab_elem_value(elem, hrec->key_size);
		475	bpf_obj_free_fields(hrec->record, map_value);
		476	}
		477
		478	static void htab_pcpu_mem_dtor(void obj, void ctx)
		479	{
		480	void __percpu pptr = (void __percpu **)obj;
		481	struct htab_btf_record *hrec = ctx;
		482	int cpu;
		483
		484	if (IS_ERR_OR_NULL(hrec->record))
		485	return;
		486
		487	for_each_possible_cpu(cpu)
		488	bpf_obj_free_fields(hrec->record, per_cpu_ptr(pptr, cpu));
		489	}
		490
		491	static void htab_dtor_ctx_free(void *ctx)
		492	{
		493	struct htab_btf_record *hrec = ctx;
		494
		495	btf_record_free(hrec->record);
		496	kfree(ctx);
		497	}
		498
		499	static int htab_set_dtor(const struct bpf_htab htab, void (dtor)(void , void ))
		500	{
		501	u32 key_size = htab->map.key_size;
		502	const struct bpf_mem_alloc *ma;
		503	struct htab_btf_record *hrec;
		504	int err;
		505
		506	/* No need for dtors. */
		507	if (IS_ERR_OR_NULL(htab->map.record))
		508	return 0;
		509
		510	hrec = kzalloc(sizeof(*hrec), GFP_KERNEL);
		511	if (!hrec)
		512	return -ENOMEM;
		513	hrec->key_size = key_size;
		514	hrec->record = btf_record_dup(htab->map.record);
		515	if (IS_ERR(hrec->record)) {
		516	err = PTR_ERR(hrec->record);
		517	kfree(hrec);
		518	return err;
		519	}
		520	ma = htab_is_percpu(htab) ? &htab->pcpu_ma : &htab->ma;
		521	/* Kinda sad, but cast away const-ness since we change ma->dtor. */
		522	bpf_mem_alloc_set_dtor((struct bpf_mem_alloc *)ma, dtor, htab_dtor_ctx_free, hrec);
		523	return 0;
		524	}
		525
		526	static int htab_map_check_btf(const struct bpf_map map, const struct btf btf,
		527	const struct btf_type key_type, const struct btf_type value_type)
		528	{
		529	struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
		530
		531	if (htab_is_prealloc(htab))
		532	return 0;
		533	/*
		534	* We must set the dtor using this callback, as map's BTF record is not
		535	* populated in htab_map_alloc(), so it will always appear as NULL.
		536	*/
		537	if (htab_is_percpu(htab))
		538	return htab_set_dtor(htab, htab_pcpu_mem_dtor);
		539	else
		540	return htab_set_dtor(htab, htab_mem_dtor);
		541	}
		542
460	static struct bpf_map htab_map_alloc(union bpf_attr attr)	543	static struct bpf_map htab_map_alloc(union bpf_attr attr)
461	{	544	{
462	bool percpu = (attr->map_type == BPF_MAP_TYPE_PERCPU_HASH \|\|	545	bool percpu = (attr->map_type == BPF_MAP_TYPE_PERCPU_HASH \|\|
@@ -2281,6 +2364,7 @@ const struct bpf_map_ops htab_map_ops = { · 1818 unmodified lines
2281	.map_seq_show_elem = htab_map_seq_show_elem,	2364	.map_seq_show_elem = htab_map_seq_show_elem,
2282	.map_set_for_each_callback_args = map_set_for_each_callback_args,	2365	.map_set_for_each_callback_args = map_set_for_each_callback_args,
2283	.map_for_each_callback = bpf_for_each_hash_elem,	2366	.map_for_each_callback = bpf_for_each_hash_elem,
		2367	.map_check_btf = htab_map_check_btf,
2284	.map_mem_usage = htab_map_mem_usage,	2368	.map_mem_usage = htab_map_mem_usage,
2285	BATCH_OPS(htab),	2369	BATCH_OPS(htab),
2286	.map_btf_id = &htab_map_btf_ids[0],	2370	.map_btf_id = &htab_map_btf_ids[0],
@@ -2303,6 +2387,7 @@ const struct bpf_map_ops htab_lru_map_ops = { · 16 unmodified lines
2303	.map_seq_show_elem = htab_map_seq_show_elem,	2387	.map_seq_show_elem = htab_map_seq_show_elem,
2304	.map_set_for_each_callback_args = map_set_for_each_callback_args,	2388	.map_set_for_each_callback_args = map_set_for_each_callback_args,
2305	.map_for_each_callback = bpf_for_each_hash_elem,	2389	.map_for_each_callback = bpf_for_each_hash_elem,
		2390	.map_check_btf = htab_map_check_btf,
2306	.map_mem_usage = htab_map_mem_usage,	2391	.map_mem_usage = htab_map_mem_usage,
2307	BATCH_OPS(htab_lru),	2392	BATCH_OPS(htab_lru),
2308	.map_btf_id = &htab_map_btf_ids[0],	2393	.map_btf_id = &htab_map_btf_ids[0],
@@ -2482,6 +2567,7 @@ const struct bpf_map_ops htab_percpu_map_ops = { · 173 unmodified lines
2482	.map_seq_show_elem = htab_percpu_map_seq_show_elem,	2567	.map_seq_show_elem = htab_percpu_map_seq_show_elem,
2483	.map_set_for_each_callback_args = map_set_for_each_callback_args,	2568	.map_set_for_each_callback_args = map_set_for_each_callback_args,
2484	.map_for_each_callback = bpf_for_each_hash_elem,	2569	.map_for_each_callback = bpf_for_each_hash_elem,
		2570	.map_check_btf = htab_map_check_btf,
2485	.map_mem_usage = htab_map_mem_usage,	2571	.map_mem_usage = htab_map_mem_usage,
2486	BATCH_OPS(htab_percpu),	2572	BATCH_OPS(htab_percpu),
2487	.map_btf_id = &htab_map_btf_ids[0],	2573	.map_btf_id = &htab_map_btf_ids[0],
@@ -2502,6 +2588,7 @@ const struct bpf_map_ops htab_lru_percpu_map_ops = { · 14 unmodified lines
2502	.map_seq_show_elem = htab_percpu_map_seq_show_elem,	2588	.map_seq_show_elem = htab_percpu_map_seq_show_elem,
2503	.map_set_for_each_callback_args = map_set_for_each_callback_args,	2589	.map_set_for_each_callback_args = map_set_for_each_callback_args,
2504	.map_for_each_callback = bpf_for_each_hash_elem,	2590	.map_for_each_callback = bpf_for_each_hash_elem,
		2591	.map_check_btf = htab_map_check_btf,
2505	.map_mem_usage = htab_map_mem_usage,	2592	.map_mem_usage = htab_map_mem_usage,
2506	BATCH_OPS(htab_lru_percpu),	2593	BATCH_OPS(htab_lru_percpu),
2507	.map_btf_id = &htab_map_btf_ids[0],	2594	.map_btf_id = &htab_map_btf_ids[0],

kernel/bpf/memalloc.c+47−11

 	int percpu_size;
 	bool draining;
 	struct bpf_mem_cache *tgt;
+	void (*dtor)(void *obj, void *ctx);
+	void *dtor_ctx;
 	/* list of objects to be freed after RCU GP */
 	struct llist_head free_by_rcu;
 	kfree(obj);
 }
-static int free_all(struct llist_node *llnode, bool percpu)
+static int free_all(struct bpf_mem_cache *c, struct llist_node *llnode, bool percpu)
 {
 	struct llist_node *pos, *t;
 	int cnt = 0;
 	llist_for_each_safe(pos, t, llnode) {
+		if (c->dtor)
+			c->dtor((void *)pos + LLIST_NODE_SZ, c->dtor_ctx);
 		free_one(pos, percpu);
 		cnt++;
 	}
 {
 	struct bpf_mem_cache *c = container_of(head, struct bpf_mem_cache, rcu_ttrace);
-	free_all(llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
+	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), !!c->percpu_size);
 	atomic_set(&c->call_rcu_ttrace_in_progress, 0);
 }
 	if (atomic_xchg(&c->call_rcu_ttrace_in_progress, 1)) {
 		if (unlikely(READ_ONCE(c->draining))) {
 			llnode = llist_del_all(&c->free_by_rcu_ttrace);
-			free_all(llnode, !!c->percpu_size);
+			free_all(c, llnode, !!c->percpu_size);
 		}
 		return;
 	}
 	dec_active(c, &flags);
 	if (unlikely(READ_ONCE(c->draining))) {
-		free_all(llist_del_all(&c->waiting_for_gp), !!c->percpu_size);
+		free_all(c, llist_del_all(&c->waiting_for_gp), !!c->percpu_size);
 		atomic_set(&c->call_rcu_in_progress, 0);
 	} else {
 		call_rcu_hurry(&c->rcu, __free_by_rcu);
 	 * Except for waiting_for_gp_ttrace list, there are no concurrent operations
 	 * on these lists, so it is safe to use __llist_del_all().
 	 */
-	free_all(llist_del_all(&c->free_by_rcu_ttrace), percpu);
+	free_all(c, llist_del_all(&c->free_by_rcu_ttrace), percpu);
-	free_all(llist_del_all(&c->waiting_for_gp_ttrace), percpu);
+	free_all(c, llist_del_all(&c->waiting_for_gp_ttrace), percpu);
-	free_all(__llist_del_all(&c->free_llist), percpu);
+	free_all(c, __llist_del_all(&c->free_llist), percpu);
-	free_all(__llist_del_all(&c->free_llist_extra), percpu);
+	free_all(c, __llist_del_all(&c->free_llist_extra), percpu);
-	free_all(__llist_del_all(&c->free_by_rcu), percpu);
+	free_all(c, __llist_del_all(&c->free_by_rcu), percpu);
-	free_all(__llist_del_all(&c->free_llist_extra_rcu), percpu);
+	free_all(c, __llist_del_all(&c->free_llist_extra_rcu), percpu);
-	free_all(llist_del_all(&c->waiting_for_gp), percpu);
+	free_all(c, llist_del_all(&c->waiting_for_gp), percpu);
 }
 static void check_mem_cache(struct bpf_mem_cache *c)
 static void free_mem_alloc_no_barrier(struct bpf_mem_alloc *ma)
 {
+	/* We can free dtor ctx only once all callbacks are done using it. */
+	if (ma->dtor_ctx_free)
+		ma->dtor_ctx_free(ma->dtor_ctx);
 	check_leaked_objs(ma);
 	free_percpu(ma->cache);
 	free_percpu(ma->caches);
 	return 0;
 }
+void bpf_mem_alloc_set_dtor(struct bpf_mem_alloc *ma, void (*dtor)(void *obj, void *ctx),
+			    void (*dtor_ctx_free)(void *ctx), void *ctx)
+{
+	struct bpf_mem_caches *cc;
+	struct bpf_mem_cache *c;
+	int cpu, i;
+	ma->dtor_ctx_free = dtor_ctx_free;
+	ma->dtor_ctx = ctx;
+	if (ma->cache) {
+		for_each_possible_cpu(cpu) {
+			c = per_cpu_ptr(ma->cache, cpu);
+			c->dtor = dtor;
+			c->dtor_ctx = ctx;
+		}
+	}
+	if (ma->caches) {
+		for_each_possible_cpu(cpu) {
+			cc = per_cpu_ptr(ma->caches, cpu);
+			for (i = 0; i < NUM_CACHES; i++) {
+				c = &cc->cache[i];
+				c->dtor = dtor;
+				c->dtor_ctx = ctx;
+			}
+		}
+	}
+}