Commit 5263e30f authored by Alexei Starovoitov's avatar Alexei Starovoitov
Browse files

Merge branch 'close-race-in-freeing-special-fields-and-map-value' 

Kumar Kartikeya Dwivedi says:

====================
Close race in freeing special fields and map value

There exists a race across various map types where the freeing of
special fields (tw, timer, wq, kptr, etc.) can be done eagerly when a
logical delete operation is done on a map value, such that the program
which continues to have access to such a map value can recreate the
fields and cause them to leak.

The set contains fixes for this case. It is a continuation of Mykyta's
previous attempt in [0], but applies to all fields. A test is included
which reproduces the bug reliably in absence of the fixes.

Local Storage Benchmarks
------------------------
Evaluation Setup: Benchmarked on a dual-socket Intel Xeon Gold 6348 (Ice
Lake) @ 2.60GHz (56 cores / 112 threads), with the CPU governor set to
performance. Bench was pinned to a single NUMA node throughout the test.

Benchmark comes from [1] using the following command:
./bench -p 1 local-storage-create --storage-type <socket,task> --batch-size <16,32,64>

Before the test, 10 runs of all cases ([socket|task] x 3 batch sizes x 7
iterations per batch size) are done to warm up and prime the machine.

Then, 3 runs of all cases are done (with and without the patch, across
reboots).

For each comparison, we have 21 samples, i.e. per batch size (e.g.
socket 16) of a given local storage, we have 3 runs x 7 iterations.

The statistics (mean, median, stddev) and t-test is done for each
scenario (local storage and batch size pair) individually (21 samples
for either case). All values are for local storage creations in thousand
creations / sec (k/s).

	       Baseline (without patch)               With patch                       Delta
     Case      Median        Mean   Std. Dev.   Median        Mean   Std. Dev.   Median       %
---------------------------------------------------------------------------------------------------
socket 16     432.026     431.941    1.047     431.347     431.953    1.635      -0.679    -0.16%
socket 32     432.641     432.818    1.535     432.488     432.302    1.508      -0.153    -0.04%
socket 64     431.504     431.996    1.337     429.145     430.326    2.469      -2.359    -0.55%
  task 16      38.816      39.382    1.456      39.657      39.337    1.831      +0.841    +2.17%
  task 32      38.815      39.644    2.690      38.721      39.122    1.636      -0.094    -0.24%
  task 64      37.562      38.080    1.701      39.554      38.563    1.689      +1.992    +5.30%

The cases for socket are within the range of noise, and improvements in task
local storage are due to high variance (CV ~4%-6% across batch sizes). The only
statistically significant case worth mentioning is socket with batch size 64
with p-value from t-test < 0.05, but the absolute difference is small (~2k/s).

TL;DR there doesn't appear to be any significant regression or improvement.

  [0]: https://lore.kernel.org/bpf/20260216131341.1285427-1-mykyta.yatsenko5@gmail.com
  [1]: https://lore.kernel.org/bpf/20260205222916.1788211-1-ameryhung@gmail.com

Changelog:
----------
v2 -> v3
v2: https://lore.kernel.org/bpf/20260227052031.3988575-1-memxor@gmail.com

 * Add syzbot Tested-by.
 * Add Amery's Reviewed-by.
 * Fix missing rcu_dereference_check() in __bpf_selem_free_rcu. (BPF CI Bot)
 * Remove migrate_disable() in bpf_selem_free_rcu. (Alexei)

v1 -> v2
v1: https://lore.kernel.org/bpf/20260225185121.2057388-1-memxor@gmail.com

 * Add Paul's Reviewed-by.
 * Fix use-after-free in accessing bpf_mem_alloc embedded in map. (syzbot CI)
 * Add benchmark numbers for local storage.
 * Add extra test case for per-cpu hashmap coverage with up to 16 refcount leaks.
 * Target bpf tree.
====================

Link: https://patch.msgid.link/20260227224806.646888-1-memxor@gmail.com


Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parents 6881af27 2939d7b3

include/linux/bpf.h

+2 −2
Original line number Diff line number Diff line
@@ -124,7 +124,7 @@ struct bpf_map_ops { 
	u32 (*map_fd_sys_lookup_elem)(void *ptr);

	void (*map_seq_show_elem)(struct bpf_map *map, void *key,

				  struct seq_file *m);

	int (*map_check_btf)(const struct bpf_map *map,

	int (*map_check_btf)(struct bpf_map *map,

			     const struct btf *btf,

			     const struct btf_type *key_type,

			     const struct btf_type *value_type);
@@ -656,7 +656,7 @@ static inline bool bpf_map_support_seq_show(const struct bpf_map *map) 
		map->ops->map_seq_show_elem;

}



int map_check_no_btf(const struct bpf_map *map,

int map_check_no_btf(struct bpf_map *map,

		     const struct btf *btf,

		     const struct btf_type *key_type,

		     const struct btf_type *value_type);

include/linux/bpf_local_storage.h

+1 −1
Original line number Diff line number Diff line
@@ -176,7 +176,7 @@ u32 bpf_local_storage_destroy(struct bpf_local_storage *local_storage); 
void bpf_local_storage_map_free(struct bpf_map *map,

				struct bpf_local_storage_cache *cache);



int bpf_local_storage_map_check_btf(const struct bpf_map *map,

int bpf_local_storage_map_check_btf(struct bpf_map *map,

				    const struct btf *btf,

				    const struct btf_type *key_type,

				    const struct btf_type *value_type);

include/linux/bpf_mem_alloc.h

+6 −0
Original line number Diff line number Diff line
@@ -14,6 +14,8 @@ struct bpf_mem_alloc { 
	struct obj_cgroup *objcg;

	bool percpu;

	struct work_struct work;

	void (*dtor_ctx_free)(void *ctx);

	void *dtor_ctx;

};



/* 'size != 0' is for bpf_mem_alloc which manages fixed-size objects.
@@ -32,6 +34,10 @@ int bpf_mem_alloc_percpu_init(struct bpf_mem_alloc *ma, struct obj_cgroup *objcg 
/* The percpu allocation with a specific unit size. */

int bpf_mem_alloc_percpu_unit_init(struct bpf_mem_alloc *ma, int size);

void bpf_mem_alloc_destroy(struct bpf_mem_alloc *ma);

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);



/* Check the allocation size for kmalloc equivalent allocator */

int bpf_mem_alloc_check_size(bool percpu, size_t size);

kernel/bpf/arena.c

+1 −1
Original line number Diff line number Diff line
@@ -303,7 +303,7 @@ static long arena_map_update_elem(struct bpf_map *map, void *key, 
	return -EOPNOTSUPP;

}



static int arena_map_check_btf(const struct bpf_map *map, const struct btf *btf,

static int arena_map_check_btf(struct bpf_map *map, const struct btf *btf,

			       const struct btf_type *key_type, const struct btf_type *value_type)

{

	return 0;

kernel/bpf/arraymap.c

+1 −1
Original line number Diff line number Diff line
@@ -548,7 +548,7 @@ static void percpu_array_map_seq_show_elem(struct bpf_map *map, void *key, 
	rcu_read_unlock();

}



static int array_map_check_btf(const struct bpf_map *map,

static int array_map_check_btf(struct bpf_map *map,

			       const struct btf *btf,

			       const struct btf_type *key_type,

			       const struct btf_type *value_type)